Fix 415 #441

samussiah · 2022-05-09T16:20:11Z

Overview

Align documentation and consistency of domain functions (AE, PD, IE, Consent)
Add data-driven data specifications to documentation.
Extend associated unit testing.

Test Notes/Sample Code

Unit test coverage extended.

Notes:

I didn't write an action to build the .md files but there is a script in the (new) dev folder that builds all the data specifications.

… finish updating domain function headers and QC functions

jwildfire

Looks good to me in general! Will leave full review and approval to @mattroumaya.

Lets file an issue to set up the action to generate the content in man/md and assign to @kodesiba

samussiah · 2022-05-10T20:27:02Z

@mattroumaya, cleaned the header documentation up a bit to assuage R CMD. I've merged dev again so this branch should be good to go.

add action and move build function to R file

mattroumaya · 2022-05-11T13:24:06Z

@samussiah just looked at the failing CI check and it's pretty confusing, only learned about this last week because @kodesiba clued me in:

Just need to add the new functions you developed to _pkgdown.yaml

- generate_md_table
- parse_data_mapping
- parse_data_spec

It looks like we could also use @keywords internal in the roxygen to omit these functions from the pkgdown site if desired: thackl/gggenomes#25

samussiah · 2022-05-11T13:54:55Z

Ayyy, it's working!

kodesiba · 2022-05-11T16:02:22Z

.github/workflows/build_markdown.yaml

@@ -0,0 +1,52 @@
+on:
+  pull_request:
+    branches: [dev, fix-415]


can you change this to just dev @samussiah, I put this in as a test so the fix-415 branch can get taken out

mattroumaya

The documentation updates and attention to detail are super impressive, awesome work!

Most things are directly noted, and apologies if being too tedious.

The only question I have is about NA values in the mapping documentation (e.g., here). I'm wondering if this is intended, and if so, maybe we should note what NA means, or instead default to replacing NA with FALSE when there isn't a specified value for a data frame.

mattroumaya · 2022-05-11T14:59:01Z

man/md/AE_Map_Adam.md

@@ -0,0 +1,9 @@
+# Data specification
+
+|domain |col_key     |col_value |vRequired |


AE_Map_Adam is missing a vUniqueCols column

mattroumaya · 2022-05-11T15:13:27Z

man/md/Consent_Map_Raw.md

+|dfSUBJ    |strIDCol       |SubjectID     |TRUE      |NA      |TRUE        |
+|dfSUBJ    |strSiteCol     |SiteID        |TRUE      |NA      |FALSE       |
+|dfSUBJ    |strRandDateCol |RandDate      |TRUE      |NA      |FALSE       |
+|dfCONSENT |strIDCol       |SubjectID     |TRUE      |FALSE   |NA          |


Should vUniqueCols for dfCONSENT be NA here?

mattroumaya · 2022-05-11T15:21:38Z

R/IE_Assess.R

+    if (any(unname(map_dbl(lTags, ~ length(.))) > 1)) {
+      lTags <- map(lTags, ~ paste(.x, collapse = ", "))


This was almost certainly my oversight from before but just caught it

Suggested change

if (any(unname(map_dbl(lTags, ~ length(.))) > 1)) {

lTags <- map(lTags, ~ paste(.x, collapse = ", "))

if (any(unname(purrr::map_dbl(lTags, ~ length(.))) > 1)) {

lTags <- purrr:::map(lTags, ~ paste(.x, collapse = ", "))

mattroumaya · 2022-05-11T15:22:37Z

R/IE_Map_Raw.R

 #'
-#' Convert from raw data format to needed input format for Inclusion/Exclusion Assessment.
+#' @description
+#' Convert raw inclusion/exclusion (IE) data, typically processed case report from data, to formatted


Suggested change

#' Convert raw inclusion/exclusion (IE) data, typically processed case report from data, to formatted

#' Convert raw inclusion/exclusion (IE) data, typically processed case report form data, to formatted

mattroumaya · 2022-05-11T15:32:21Z

man/md/IE_Map_Raw.md

+|:------|:--------------|:-----------|:---------|:-----------|
+|dfSUBJ |strIDCol       |SubjectID   |TRUE      |TRUE        |
+|dfSUBJ |strSiteCol     |SiteID      |TRUE      |FALSE       |
+|dfIE   |strIDCol       |SubjectID   |TRUE      |NA          |


Should there be NA cols?

mattroumaya · 2022-05-11T15:44:31Z

R/IE_Assess.R

+#'   - `dfFlagged`, returned by {gsm::dfFlagged()}
+#'   - `dfSummary`, returned by {gsm::dfSummary()}


Suggested change

#' - `dfFlagged`, returned by {gsm::dfFlagged()}

#' - `dfSummary`, returned by {gsm::dfSummary()}

#' - `dfFlagged`, returned by {gsm::Flag()}

#' - `dfSummary`, returned by {gsm::Summarize()}

mattroumaya · 2022-05-11T18:19:16Z

man/md/AE_Map_Raw.md

+|dfSUBJ |strIDCol              |SubjectID       |TRUE      |TRUE        |
+|dfSUBJ |strSiteCol            |SiteID          |TRUE      |FALSE       |
+|dfSUBJ |strTimeOnTreatmentCol |TimeOnTreatment |TRUE      |FALSE       |
+|dfAE   |strIDCol              |SubjectID       |TRUE      |NA          |


NA here too - I'll leave a top-level comment about addressing this but is likely an easy-ish solution or something that we don't need to worry about

mattroumaya · 2022-05-11T18:19:34Z

man/md/PD_Map_Raw.md

+|dfSUBJ |strIDCol          |SubjectID   |TRUE      |TRUE        |
+|dfSUBJ |strSiteCol        |SiteID      |TRUE      |FALSE       |
+|dfSUBJ |strTimeOnStudyCol |TimeOnStudy |TRUE      |FALSE       |
+|dfPD   |strIDCol          |SubjectID   |TRUE      |NA          |


Just noting NA here

mattroumaya · 2022-05-11T18:27:52Z

tests/testthat/test_Consent_Assess.R

@@ -54,7 +54,19 @@ test_that("incorrect lTags throw errors",{


 # custom tests ------------------------------------------------------------
+test_that('dfAnalyzed has appropriate model output regardless of statistical method', {
+    assessment <- Consent_Assess(consentInput)
+    expect_true(all(c('Estimate') %in% names(assessment$dfAnalyzed)))


Suggested change

expect_true(all(c('Estimate') %in% names(assessment$dfAnalyzed)))

expect_true('Estimate' %in% names(assessment$dfAnalyzed)))

Just being very tedious here. could also be:

expect_true(hasName(assessment$dfAnalyzed, 'Estimate'))

mattroumaya · 2022-05-11T18:28:28Z

tests/testthat/test_IE_Assess.R

@@ -51,7 +51,19 @@ test_that("incorrect lTags throw errors",{
 })

 # custom tests ------------------------------------------------------------
+test_that('dfAnalyzed has appropriate model output regardless of statistical method', {
+    assessment <- IE_Assess(ieInput)
+    expect_true(all(c('Estimate') %in% names(assessment$dfAnalyzed)))


Same here but feel free to ignore

mattroumaya

Looks great!

mattroumaya · 2022-05-12T15:26:02Z

R/IE_Map_Raw.R

-    return(list(dfInput=dfInput, lChecks=checks))
-  }else{
+  if (bReturnChecks) {
+    return(list(df = dfInput, lChecks = checks))


good catch!

Spencer Childress added 6 commits May 4, 2022 16:37

add .md files for each assessment

2d73348

get .md files working in function headers

172fc2c

add script that produces all data specification markdown files; TODO:…

8794d59

… finish updating domain function headers and QC functions

fix #415; begin QC of domain-specific functions

f49c6f8

update AE_Map_Adam.R tests

c78d906

fix #411

01fb30d

jwildfire marked this pull request as draft May 10, 2022 14:34

fix #411; fix #416; fix #417; fix #418

5da1367

samussiah requested review from mattroumaya and jwildfire May 10, 2022 14:51

samussiah marked this pull request as ready for review May 10, 2022 14:54

merge dev and rebuild documentation

54e5d81

jwildfire reviewed May 10, 2022

View reviewed changes

This was linked to issues May 10, 2022

QC: AE Functions for v0.4.1 #411

Closed

QC: Consent Functions for v0.4.1 #416

Closed

QC: IE Functions for v0.4.1 #417

Closed

QC: PD Functions for v0.4.1 #418

Closed

QC: update documentation - function description #376

Closed

mattroumaya added this to the v0.4.1 milestone May 10, 2022

mattroumaya assigned samussiah May 10, 2022

Nathan Kosiba and others added 6 commits May 10, 2022 17:12

add action and move build function to R file

a55d2da

update commit stuff

6e5ecc7

remove some files for test

4f25cc4

Build Markdown

4e05ca9

generate markdown tables as opposed to html tables

0367dcf

merge dev

241b73d

samussiah requested a review from jwildfire May 10, 2022 20:25

Spencer Childress and others added 2 commits May 10, 2022 16:30

Merge branch 'fix-415' into fix-415_action

baa8ee5

Merge pull request #445 from Gilead-BioStats/fix-415_action

6e9f729

add action and move build function to R file

updates test snapshots

6f2e96e

update _pkgdown.yml

0eba265

kodesiba reviewed May 11, 2022

View reviewed changes

mattroumaya reviewed May 11, 2022

View reviewed changes

mattroumaya linked an issue May 11, 2022 that may be closed by this pull request

QC: update roxygen to document data specs/yaml for each assessment #415

Closed

address @mattroumaya's review comments

4aff073

samussiah requested a review from mattroumaya May 12, 2022 14:58

mattroumaya approved these changes May 12, 2022

View reviewed changes

mattroumaya merged commit 6257b85 into dev May 12, 2022

mattroumaya deleted the fix-415 branch May 12, 2022 15:41

mattroumaya linked an issue May 13, 2022 that may be closed by this pull request

Bugfix: Remove fix-415 from styler workflow YAML. #447

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 415 #441

Fix 415 #441

samussiah commented May 9, 2022 •

edited

Loading

jwildfire left a comment

samussiah commented May 10, 2022

mattroumaya commented May 11, 2022

samussiah commented May 11, 2022

kodesiba May 11, 2022

mattroumaya left a comment

mattroumaya May 11, 2022

mattroumaya May 11, 2022

mattroumaya May 11, 2022

mattroumaya May 11, 2022

mattroumaya May 11, 2022

mattroumaya May 11, 2022

mattroumaya May 11, 2022

mattroumaya May 11, 2022

mattroumaya May 11, 2022

mattroumaya May 11, 2022

mattroumaya left a comment

mattroumaya May 12, 2022

		@@ -0,0 +1,9 @@
		# Data specification

		\|domain \|col_key \|col_value \|vRequired \|

		if (any(unname(map_dbl(lTags, ~ length(.))) > 1)) {
		lTags <- map(lTags, ~ paste(.x, collapse = ", "))

	#' Convert raw inclusion/exclusion (IE) data, typically processed case report from data, to formatted
	#' Convert raw inclusion/exclusion (IE) data, typically processed case report form data, to formatted

		#' - `dfFlagged`, returned by {gsm::dfFlagged()}
		#' - `dfSummary`, returned by {gsm::dfSummary()}

	expect_true(all(c('Estimate') %in% names(assessment$dfAnalyzed)))
	expect_true('Estimate' %in% names(assessment$dfAnalyzed)))

Fix 415 #441

Fix 415 #441

Conversation

samussiah commented May 9, 2022 • edited Loading

Overview

Test Notes/Sample Code

jwildfire left a comment

Choose a reason for hiding this comment

samussiah commented May 10, 2022

mattroumaya commented May 11, 2022

samussiah commented May 11, 2022

Choose a reason for hiding this comment

mattroumaya left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattroumaya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samussiah commented May 9, 2022 •

edited

Loading