Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix #566 - Faceted charts for stratified analysis #567

Merged
merged 24 commits into from
Aug 4, 2022
Merged

Fix #566 - Faceted charts for stratified analysis #567

merged 24 commits into from
Aug 4, 2022

Conversation

jwildfire
Copy link
Contributor

@jwildfire jwildfire commented Jun 24, 2022 •
edited by samussiah
Loading

Overview

Simple implementation of faceting for stratified analysis. Need to think about workflow updates to best implement in reports.

Test Notes/Sample Code

aegrade <- Study_Assess(lAssessments = list(aeGrade = MakeAssessmentList()$aeGrade))
allFlagged <- aegrade %>% 
    imap(function(assessment, name){
        flagged <- assessment$lResults$dfFlagged %>% mutate(strata=name)
    })%>%bind_rows
dfBounds <- aegrade %>%
    imap(function(assessment, name) {
        assessment$lResults$dfBounds %>% mutate(strata = name)
    }) %>%
    bind_rows
Visualize_Scatter(allFlagged, strGroupCol="strata", dfBounds = dfBounds)

image

@jwildfire jwildfire marked this pull request as draft June 24, 2022 15:52
@jwildfire jwildfire self-assigned this Jun 24, 2022
@samussiah samussiah requested review from samussiah and removed request for samussiah June 28, 2022 14:40
@jwildfire jwildfire linked an issue Jul 15, 2022 that may be closed by this pull request
Feature: Add paneled visualization of stratified KRI results #566
Closed
@jwildfire
Copy link
Contributor Author

@samussiah to add a stratified PD Chart.

@samussiah
Copy link
Contributor

PD chart - next up adding the stratification to the workflow.
image

@samussiah
Copy link
Contributor

I'm leaning toward adding a RunStratifiedWorkflow() function that takes as input the output from the individual strata.

@samussiah samussiah requested a review from mattroumaya August 3, 2022 15:34
@samussiah samussiah assigned samussiah and unassigned jwildfire Aug 3, 2022
@samussiah samussiah requested a review from kodesiba August 3, 2022 15:34
@samussiah samussiah marked this pull request as ready for review August 3, 2022 15:35
@jwildfire jwildfire mentioned this pull request Aug 3, 2022
Closed
@mattroumaya
Copy link
Contributor

@samussiah - Is this ready for review? Happy to start one but just wanted to check since some of the CI is currently failing.

@samussiah
Copy link
Contributor

Yeah, ready for review @mattroumaya.

mattroumaya
mattroumaya approved these changes Aug 4, 2022
View reviewed changes
Copy link
Contributor

@mattroumaya mattroumaya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work here @samussiah! I learned some new things from this review and think it looks great. Just added some minor comments but this looks good to merge after you review the suggested changes.

R/Study_Assess.R
)
lAssessments <- lAssessments %>%
map(function(lAssessment) {
Runction <- ifelse(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😄 💯

R/Visualize_Count.R
@@ -21,7 +22,7 @@
#'
#' @export

Visualize_Count <- function(dfAnalyzed, strTotalCol = "N", strCountCol = "TotalCount", strTitle = "") {
Visualize_Count <- function(dfAnalyzed, strGroupCol=NULL, strTotalCol = "N", strCountCol = "TotalCount", strTitle = "") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add to stopifnot() below:

"strGroupCol must be character" = is.character(strGroupCol),
"strGroupCol not found in dfAnalyzed" = strGroupCol %in% names(dfAnalyzed)

R/Visualize_Scatter.R
#' @import ggplot2
#'
#' @export

Visualize_Scatter <- function(dfFlagged, dfBounds = NULL, strUnit = "days") {
Visualize_Scatter <- function(dfFlagged, dfBounds = NULL, strGroupCol = NULL, strUnit = "days") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add to stopifnot() below:

"strGroupCol must be character" = is.character(strGroupCol),
"strGroupCol not found in dfAnalyzed" = strGroupCol %in% names(dfAnalyzed)

R/util-ConsolidateStrata.R Outdated Show resolved Hide resolved
R/util-MakeStratifiedAssessment.R
domainName <- lWorkflow$group$domain
data <- lData[[ domainName ]]
columnName <- lMapping[[ domainName ]][[ lWorkflow$group$columnParam ]]
strata <- data[[ columnName ]] %>% unique %>% sort
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure of the implications for workflow/how data will be passed to ZS, so this might be irrelevant.

Do we want to wrap this in if(length(strata) > 1? Or maybe add a stopifnot() requirement that the number of unique data[[ columnName ]] > 1?

It does still work with a single unique strata value and returns:

Stratified workflow created for each level of dfPD$DVDECOD (n=1).

with a list value of StratifiedPD$pdCategory_1

Just noting this to avoid confusion to the end-user if they're expecting a stratified result and receive a single strata.

R/util-MakeStratifiedAssessment.R Outdated Show resolved Hide resolved
R/util-RunStratifiedWorkflow.R Outdated Show resolved Hide resolved
R/util-MakeStratifiedAssessment.R Outdated Show resolved Hide resolved
mattroumaya and others added 8 commits August 4, 2022 17:40
@mattroumaya
update unit tests
3b5b48b
@mattroumaya
update param docs
73ff71f
@samussiah @mattroumaya
Update R/util-ConsolidateStrata.R
bc2b62f 
Co-authored-by: Matt Roumaya <40671730+mattroumaya@users.noreply.github.com>
@samussiah @mattroumaya
Update R/util-MakeStratifiedAssessment.R
14e116e 
Co-authored-by: Matt Roumaya <40671730+mattroumaya@users.noreply.github.com>
@samussiah @mattroumaya
Update R/util-MakeStratifiedAssessment.R
f679ed2 
Co-authored-by: Matt Roumaya <40671730+mattroumaya@users.noreply.github.com>
@samussiah @mattroumaya
Update R/util-RunStratifiedWorkflow.R
3a54f15 
Co-authored-by: Matt Roumaya <40671730+mattroumaya@users.noreply.github.com>
@mattroumaya
update pkgdown index
46690d2
@mattroumaya
ffwd
cae7e72 
Merge branch 'dev' of github.com:Gilead-BioStats/gsm into fix-566

# 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 mattroumaya merged commit 138b76a into dev Aug 4, 2022
@mattroumaya mattroumaya deleted the fix-566 branch August 4, 2022 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattroumaya mattroumaya mattroumaya approved these changes

@kodesiba kodesiba Awaiting requested review from kodesiba

Assignees

@samussiah samussiah

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature: Add paneled visualization of stratified KRI results
3 participants
@jwildfire @samussiah @mattroumaya