Gilead-BioStats / gsm

Good Statistical Monitoring R Package
https://gilead-biostats.github.io/gsm/
Apache License 2.0
36 stars 9 forks source link

Fix sampleData #1661

Closed lauramaxwell closed 1 month ago

lauramaxwell commented 1 month ago

Overview

replaces sampleSummary data.frame per the specifications in #1651 and updates sampleMetrics$GroupLevel to "Site" to match formatting across package.

closes #1651

Test Notes/Sample Code

Notes: PR #1657 should fix pipeline when merged in

lauramaxwell commented 1 month ago

@jwildfire One note/question about this update: sampleResults and sampleBounds aren't based on the same data, so if you try to make a charts and a report like below the charts look off.

metrics<- unique(sampleResults$MetricID)
charts <- metrics %>% map(~Visualize_Metric(
  dfResults = sampleResults,
  dfBounds = sampleBounds,
  dfGroups = sampleGroups,
  dfMetrics = sampleMetrics,
  strMetricID = .x
)
) %>% setNames(metrics)

Report_KRI(
  lCharts = charts,
  dfResults = sampleResults,
  dfGroups = sampleGroups,
  dfMetrics = sampleMetrics,
  strOutpath = "~/test.html"
)

image

Originally, sampleBounds was intended to jive with sampleSummary since they are part of the analysis data pipeline and only represent a single KRI and snapshot. thinking about it more, it might make sense for sampleBounds to be based on the data used in sampleResults, since the reporting pipeline is more likely where sampleBounds will be used in examples, etc.

jonthegeek commented 1 month ago

I'd rather have the working tests from #1657 before we review this. I suspect some things will break with this change (or, well, properly become failing tests because they were relying on a bad version of sampleSummary), and I want to be able to see what broke because of this/whether we missed any fixes here. In general (and I didn't follow this perfectly; I should have made a separate PR just for the fixes) I'd really like to try to keep all of the checks green so we can see if a new thing breaks something in a way we didn't expect!

Edit: Nevermind, you got them working here 🎉

jonthegeek commented 1 month ago

Let's get tests to pass and then we can merge this, and reconcile any remaining check failures over in #1657.

lauramaxwell commented 1 month ago

tests are passing now

jonthegeek commented 1 month ago

Gonna do an actual review of the code here now, but if we split this comment off into an issue or discussion, I think we can probably merge this after I do!

jonthegeek commented 1 month ago

sampleBounds discussion moved to #1662. The rest of this looks good!