Conversation
- non-peers are shown as transparent points - changes the logic of the u-prime calculation to be a function which returns the stuff needed to do the calculations - can then use this object to use geom_function, rather than discrete points in the funnel lines
There was a problem hiding this comment.
Pull request overview
This PR refactors the rates funnel plotting to use U‑Prime calculations, updates the funnel and box plots to display all providers while visually emphasising peers and the selected provider, and introduces more flexible y‑axis label formatting for trend plots. It also aligns several exported function interfaces and their documentation with current usage.
Changes:
- Replace the old
generate_rates_funnel_data()data-preparation path with a newuprime_calculations()helper and updated funnel plotting logic that draws U‑Prime control lines. - Update the Shiny modules and trend/funnel plotting functions to use configuration-driven y‑axis label formatters and to show all providers with peers/provider highlighted via colour/alpha.
- Simplify Azure input handling by moving container acquisition inside
get_all_geo_data()and partially updating documentation forget_container().
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
R/utils_plot.R |
Adds uprime_calculations() for computing U‑Prime control limits and adjusts baseline data preparation to keep all providers for plotting. |
R/fct_plots.R |
Updates plot_rates_trend() to accept a y_labels formatter and reworks plot_rates_funnel()/plot_rates_box() to use U‑Prime calculations and alpha/colour to highlight peers vs non‑peers. |
R/mod_plot_rates.R |
Wires in uprime_calculations() as a reactive, derives y‑axis limits from filtered baseline data and trend rates, and passes y‑label formatters and new funnel inputs into the child modules. |
R/mod_plot_rates_funnel.R |
Updates the server module to accept funnel calculations in addition to rates and pass them through to plot_rates_funnel(). |
NAMESPACE |
Removes the export for the deleted generate_rates_funnel_data() and exports the new uprime_calculations() function. |
man/uprime_calculations.Rd |
Documents the new uprime_calculations() helper for U‑Prime funnel charts. |
man/plot_rates_trend.Rd |
Documents the new y_labels argument used to format y‑axis labels in trend plots. |
man/plot_rates_funnel.Rd |
Updates the funnel plot documentation to reflect the new funnel_calculations argument and simpler rates_funnel_data input. |
man/get_container.Rd |
Updates usage for get_container() to match the current function signature, though arguments still list obsolete parameters. |
man/get_all_geo_data.Rd |
Aligns get_all_geo_data() documentation with its simplified interface that constructs the Azure container internally. |
Comments suppressed due to low confidence (1)
R/mod_plot_rates.R:83
generate_rates_funnel_data()is still being called here, but the function has been removed fromR/utils_plot.Rand is no longer exported inNAMESPACE, so this reactive will throw an error as soon as it is evaluated. Either reintroducegenerate_rates_funnel_data()or update this reactive (and any callers) to use the newuprime_calculations()output directly, removing this now-unusedrates_funnel_datareactive if it is no longer needed.
rates_funnel_data <- shiny::reactive({
shiny::req(rates_baseline_data())
rates_baseline_data() |> generate_rates_funnel_data()
})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| \item{tenant}{Character. Tenant ID.} | ||
|
|
||
| \item{app_id}{Character. App ID.} |
There was a problem hiding this comment.
The get_container() Rd lists tenant and app_id as arguments here, but the function signature now only takes ep_uri and container_name, so these parameters are no longer valid for callers and the docs are misleading. Consider removing these items (and the corresponding roxygen @param tags) so that the documented interface matches the actual function definition.
| \item{tenant}{Character. Tenant ID.} | |
| \item{app_id}{Character. App ID.} |
matt-dray
left a comment
There was a problem hiding this comment.
Great, thanks for doing the legwork on this. You screenshared this and I can reproduce it locally as expected.
I've added minor suggestions: black dashed lines for plot, some inline comments.
I'll let you react to Copilot's thoughts as you see fit.
tomjemmett
left a comment
There was a problem hiding this comment.
should remove this unused code
Co-authored-by: Matt Dray <18232097+matt-dray@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
4a2fd87 to
96312bc
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
man/uprime_calculations.Rd:12
- The argument name documented here (
rates_baseline_data) no longer matches the function signature in\usage{}, which now usesdf; this mismatch can confuse users and should be updated so the\arguments{}entry uses the same name as the actual parameter.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
matt-dray
left a comment
There was a problem hiding this comment.
Cracking, thanks for incorporating these changes. Looks good to me locally.
shows all providers in the funnel chart. non-peers are shown as a transparent dot, leaving the peers/provider as highlighted points