Fix #462 - capture KRI and other associated metadata #527

mattroumaya · 2022-06-07T21:45:57Z

Overview

Only ready for a preliminary review to make sure things are going as planned:

Tested passing KRILabel in Study_Assess() with ae.yaml
Can also pass custom KRILabel via lTags if running single assessment (see below)

Test Notes/Sample Code

# run multiple (this branch is only currently testing ae)
myStudy <- Study_Assess()
View(myStudy$ae$lResults$dfSummary)

# can pass custom KRILabel via lTags by using explicit "KRILabel" name:
ae <- AE_Map_Raw()
ae_assess <- AE_Assess(ae, lTags = list(KRILabel = "AEs/Site per Month for West Coast"))

Merge branch 'dev' into fix-462 # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

mattroumaya · 2022-06-07T21:47:04Z

@jwildfire - This should be good for your preliminary review to make sure the new skeleton is as intended. Definitely happy to schedule a time to run through everything if that's more convenient.

R/Analyze_Wilcoxon.R

jwildfire · 2022-06-08T00:44:23Z

R/AE_Assess.R

@@ -127,13 +127,13 @@ AE_Assess <- function(
        )
      }

-      lAssess$dfAnalyzed <- gsm::Analyze_Wilcoxon(lAssess$dfTransformed, "Rate", bQuiet = bQuiet)
+      lAssess$dfAnalyzed <- gsm::Analyze_Wilcoxon(lAssess$dfTransformed, "KRI", bQuiet = bQuiet)


As noted below, we can update the default to KRI and remove here.

R/Analyze_Poisson.R

R/Consent_Assess.R

R/Flag.R

jwildfire · 2022-06-08T01:02:56Z

R/IE_Assess.R

@@ -14,6 +14,7 @@
 #' @param dfInput `data.frame` Input data, a data frame with one record per subject.
 #' @param nThreshold `numeric` Threshold specification. Default: `0.5`
 #' @param lTags `list` Assessment tags, a named list of tags describing the assessment that defaults to `list(Assessment="IE")`. `lTags` is returned as part of the assessment (`lAssess$lTags`) and each tag is added as a column in `lAssess$dfSummary`.
+#' @param strScoreLabel Optional. `character` vector to describe the `Score` column. Default: `Total Event Count`


Should standarize across assessments.

R/Summarize.R

jwildfire · 2022-06-08T01:07:36Z

@mattroumaya This looks good so far. Added a bunch of minor comments, but not 100% sure about a few of the details.

I want to think a bit more about the best way to define KRILabel and ScoreLabel as part of each assessment. I think I like the logic using lTags from your example above:

# can pass custom KRILabel via lTags by using explicit "KRILabel" name:
ae <- AE_Map_Raw()
ae_assess <- AE_Assess(ae, lTags = list(KRILabel = "AEs/Site per Month for West Coast"))

Not quite sure how this works right now looking at the code though ... I'm also debating if there's a compelling reason to bury these parameters in lTags instead of just defining them directly in the Assess parameters (like you did in Consent_Assess and IE_Assess ... So maybe I'm talking my self out of the lTags approach ...

Also thinking ScoreLabel might just be hardcoded in the Analyze functions, while KRI label might be more variable (and thus defined as part of the assessment workflow.

Long story short, we should probably find another 30 minutes to discuss. I'll try to block a time tomorrow. Thanks for getting the draft PR moving, it's definitely helping us get to the needed level of detail.

jwildfire · 2022-06-08T01:47:57Z

@mattroumaya I just updated #462 and created #528 to outline my proposed approach. Feel free to add comments/questions in those issues. Or if the proposal in the issues is all fairly clear and makes sense to you feel free to try a draft implementation here.

Consider all of my comments from the code review as loose suggestions and lower priority than the comments in the issues.

Co-authored-by: Jeremy Wildfire <jwildfire@gmail.com>

mattroumaya · 2022-06-08T22:35:18Z

@kodesiba - I hope this is now ready for you to branch off of to update qualification tests. Feel free to branch off and of course let me know if you have any questions. Still some updates needed for documentation and non-quali unit tests, but shouldn't affect your updates.

R/Analyze_Chisq.R

R/Analyze_Fisher.R

R/Analyze_Identity.R

jwildfire · 2022-06-09T13:44:27Z

R/Analyze_Poisson.R

-    rename(
-      Residuals = .data$.resid,
-      PredictedCount = .data$.fitted,
+    mutate(


@gwu05 Do we want to add a strScoreCol parameter here? We can set default to ".resid", but could also allow users to set up assessments that use pvalue (or another parameter) as the Score column for use in flagging.

Let's leave this hardcoded for now. Can revisit as needed if a new use case comes up.

R/Analyze_Wilcoxon.R

R/Flag.R

jwildfire · 2022-06-09T14:01:37Z

R/Analyze_Identity.R

+#'
+#' @export
+
+Analyze_Identity <- function(dfTransformed, strValueCol = 'KRI', strLabelCol = "KRIColumn"){


Let's add bQuiet just to be thorough. Will likely add some reporting in analyze functions in future releases.

jwildfire · 2022-06-09T14:08:36Z

R/Transform_EventCount.R

@@ -27,18 +27,19 @@
 #' @param dfInput A data.frame with one record per person.
 #' @param strCountCol Required. Numerical or logical. Column to be counted.
 #' @param strExposureCol Optional. Numerical `Exposure` column.
+#' @param strKRILabel Optional. Character vector to describe the `KRI` column.


Does it make sense to have this optional? Does anything in StudyAssess break when KRILabel/ScoreLabel is missing.

I think I'd lean towards giving it a placeholder default like strKRILabel = "[Not Specified]"

This one actually isn't optional - every *_Assess() function has its own default strKRILabel, so if not specified in a workflow mapping, Study_Assess() will use the default for *_Assess().

Do you feel good about this setup? Or would you prefer putting a placeholder in Transform_EventCount() (like above), and removing default strKRILabel from Assess?

I think we should do both. Pretty unlikely this will be called outside an _Assess function, but would be good to have a reasonable default just in case.

jwildfire · 2022-06-09T14:24:57Z

tests/testthat/test-Analyze_Identity.R

@@ -0,0 +1,3 @@
+test_that("multiplication works", {


Reminder to update

jwildfire · 2022-06-09T14:26:02Z

tests/testthat/test_AE_Assess.R

@@ -65,10 +66,10 @@ test_that("incorrect lTags throw errors", {

 # custom tests ------------------------------------------------------------
 test_that("dfAnalyzed has appropriate model output regardless of statistical method", {


Might be worth adding a test to insure that Score/ScoreLabel populate as expected as well.

jwildfire · 2022-06-09T14:27:12Z

tests/testthat/test_Consent_Assess.R

@@ -70,3 +71,8 @@ test_that("bReturnChecks works as intended", {
    "lChecks" %in% names(Consent_Assess(consentInput, bReturnChecks = TRUE))
  )
 })
+
+test_that("strKRILabel works as intended", {


Add test for Score/ScoreLabel

applies to all Assess functions

jwildfire · 2022-06-09T14:29:00Z

@mattroumaya Core structure looks good! Suggested lots of minor tweaks/cleanup in latest review.

Co-authored-by: Jeremy Wildfire <jwildfire@gmail.com>

Merge branch 'dev' of github.com:Gilead-BioStats/gsm into fix-462 # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

mattroumaya · 2022-06-09T21:28:47Z

@kodesiba giving the go ahead to update qualification tests. I think everything else is covered.

@jwildfire feel free to give this another review if you have the chance, or feel free to wait until Nathan's PR is merged.

Merge branch 'dev' of github.com:Gilead-BioStats/gsm into fix-462 # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

update qualification tests for KRI update

jwildfire

💯

R/Analyze_Fisher.R

jwildfire · 2022-06-13T12:03:49Z

@mattroumaya Looks great. Will leave merge to you.

Co-authored-by: Jeremy Wildfire <jwildfire@gmail.com>

mattroumaya added 4 commits June 6, 2022 20:33

change Rate to KRI

5936af9

ffwd dev

ea8595b

Merge branch 'dev' into fix-462 # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

add KRI and Score cols

4f66741

test KRILabel with ae.yaml

eddaa44

jwildfire reviewed Jun 8, 2022

View reviewed changes

mattroumaya and others added 13 commits June 8, 2022 13:56

dont re-bind KRILabel

2a564d8

Update R/Analyze_Poisson.R

4716b30

Co-authored-by: Jeremy Wildfire <jwildfire@gmail.com>

Update R/Analyze_Poisson.R

105aa14

Co-authored-by: Jeremy Wildfire <jwildfire@gmail.com>

Update R/Consent_Assess.R

7c7b85d

Co-authored-by: Jeremy Wildfire <jwildfire@gmail.com>

Update R/Consent_Assess.R

7dfba04

Co-authored-by: Jeremy Wildfire <jwildfire@gmail.com>

Update R/Consent_Assess.R

e5886fb

Co-authored-by: Jeremy Wildfire <jwildfire@gmail.com>

Update R/Flag.R

d7f4e37

Co-authored-by: Jeremy Wildfire <jwildfire@gmail.com>

Update R/Summarize.R

909dddc

Co-authored-by: Jeremy Wildfire <jwildfire@gmail.com>

updates based on PR review

d663dd6

add Analyze_Identity + placeholder unit test

a7220a8

remove "KRI" because it is now default

3fcf712

remove labels from inst/assessments YAML files

3574402

update unit tests + documentation

c192b6d

mattroumaya mentioned this pull request Jun 8, 2022

updates to data pipeline vignette. #526

Merged

jwildfire self-requested a review June 9, 2022 13:36

jwildfire reviewed Jun 9, 2022

View reviewed changes

R/Analyze_Chisq.R Outdated Show resolved Hide resolved

jwildfire requested changes Jun 9, 2022

View reviewed changes

mattroumaya and others added 3 commits June 9, 2022 10:36

Update R/Analyze_Chisq.R

b471d08

Co-authored-by: Jeremy Wildfire <jwildfire@gmail.com>

Update R/Analyze_Fisher.R

bd9b51b

Co-authored-by: Jeremy Wildfire <jwildfire@gmail.com>

Update R/Analyze_Identity.R

bc96160

Co-authored-by: Jeremy Wildfire <jwildfire@gmail.com>

mattroumaya and others added 11 commits June 9, 2022 10:37

Update R/Analyze_Wilcoxon.R

9a3a306

Co-authored-by: Jeremy Wildfire <jwildfire@gmail.com>

Update R/Flag.R

c23fabb

Co-authored-by: Jeremy Wildfire <jwildfire@gmail.com>

Update R/Flag.R

d1a719c

Co-authored-by: Jeremy Wildfire <jwildfire@gmail.com>

Update R/IE_Assess.R

b7113e8

Co-authored-by: Jeremy Wildfire <jwildfire@gmail.com>

Update R/IE_Assess.R

8591393

Co-authored-by: Jeremy Wildfire <jwildfire@gmail.com>

[[]] in Analyze_Identity()

b32100d

unit tests

73337b1

update tests

c8f338d

update unit tests and documentation

e25741c

update examples

5b520eb

Nathan Kosiba and others added 5 commits June 10, 2022 16:45

update qualification tests for KRI update

e31e7b8

report sort

f54bae9

Merge pull request #535 from Gilead-BioStats/fix-462_quali

112aaba

update qualification tests for KRI update

add Analyze_Identity() to pkgdown index

4334b2f

mattroumaya requested a review from jwildfire June 10, 2022 18:06

jwildfire marked this pull request as ready for review June 13, 2022 11:46

jwildfire approved these changes Jun 13, 2022

View reviewed changes

R/Analyze_Fisher.R Outdated Show resolved Hide resolved

Update R/Analyze_Fisher.R

00d2aad

Co-authored-by: Jeremy Wildfire <jwildfire@gmail.com>

mattroumaya merged commit bbdb37e into dev Jun 13, 2022

mattroumaya deleted the fix-462 branch June 13, 2022 13:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #462 - capture KRI and other associated metadata #527

Fix #462 - capture KRI and other associated metadata #527

mattroumaya commented Jun 7, 2022

mattroumaya commented Jun 7, 2022

jwildfire Jun 8, 2022

jwildfire Jun 8, 2022

jwildfire commented Jun 8, 2022 •

edited

Loading

jwildfire commented Jun 8, 2022

mattroumaya commented Jun 8, 2022 •

edited

Loading

jwildfire Jun 9, 2022

jwildfire Jun 9, 2022

jwildfire Jun 9, 2022

jwildfire Jun 9, 2022

mattroumaya Jun 9, 2022

jwildfire Jun 9, 2022

jwildfire Jun 9, 2022

jwildfire Jun 9, 2022

jwildfire Jun 9, 2022

jwildfire Jun 9, 2022

jwildfire commented Jun 9, 2022 •

edited

Loading

mattroumaya commented Jun 9, 2022

jwildfire left a comment

jwildfire commented Jun 13, 2022

		@@ -65,10 +66,10 @@ test_that("incorrect lTags throw errors", {

		# custom tests ------------------------------------------------------------
		test_that("dfAnalyzed has appropriate model output regardless of statistical method", {

Fix #462 - capture KRI and other associated metadata #527

Fix #462 - capture KRI and other associated metadata #527

Conversation

mattroumaya commented Jun 7, 2022

Overview

Test Notes/Sample Code

mattroumaya commented Jun 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwildfire commented Jun 8, 2022 • edited Loading

jwildfire commented Jun 8, 2022

mattroumaya commented Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwildfire commented Jun 9, 2022 • edited Loading

mattroumaya commented Jun 9, 2022

jwildfire left a comment

Choose a reason for hiding this comment

jwildfire commented Jun 13, 2022

jwildfire commented Jun 8, 2022 •

edited

Loading

mattroumaya commented Jun 8, 2022 •

edited

Loading

jwildfire commented Jun 9, 2022 •

edited

Loading