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.

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

add option for generating guest function outputs for residualVsObserv… #176

Merged

Conversation

abdullahhamadeh
Copy link
Member

At present the DDI plot Guest bounds are for the predictedVsObserved case.

This PR allows for the residualVsObserved case.

@codecov-commenter
Copy link

codecov-commenter commented Oct 9, 2021

Codecov Report

Merging #176 (88c2bb6) into develop (3ce8490) will decrease coverage by 0.17%.
The diff coverage is 7.69%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #176      +/-   ##
===========================================
- Coverage    48.41%   48.24%   -0.18%     
===========================================
  Files           55       55              
  Lines         2551     2560       +9     
===========================================
  Hits          1235     1235              
- Misses        1316     1325       +9     
Impacted Files Coverage Δ
R/ddiratio-datamapping.R 0.00% <0.00%> (ø)
R/plot-ddiratio.R 0.00% <0.00%> (ø)
R/aaa-utilities.R 68.18% <100.00%> (ø)
R/plot-pkratio.R 93.33% <0.00%> (ø)
R/plot-boxwhisker.R 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ce8490...88c2bb6. Read the comment docs.

# Apply log10 transformation to lines because
# plot is log scaled in by default and geom_abline
# requires the log transformed values in input of intercept
self$lines <- lapply(lines, log10)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does default value in DefaultDataMappingValues include the log10 ?
If so, line 27 may apply twice the log.
This means that either we need lapply(lines, log10) %||% DefaultDataMappingValues$obsVsPred with lines = NULL in input or that DefaultDataMappingValues account for scale somehow

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we may want to apply the log10 directly within the plotXX functions after checking the plotConfiguration scale is included in Scaling$log (obs vs pred and res vs pred might be in linear scale).
Otherwise, users can get unwanted log transformation of their diagonal/horizontal lines.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If possible could you document the scaling in the @param lines and maybe in the pk-ratio vignette

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverting changes to obs-vs-pred-datamapping.R and pkratio-datamapping.R for now.

Comment on lines 28 to 30
if(dataMapping$residualsVsObserved){
lineOrientation <- "horizontal"
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a shortcut option for users in the function input as well: e.g. residuals = NULL ?
and internally do something like

validateIsLogical(residuals, nullAllowed = TRUE)
residuals <- residuals %||% dataMapping$residualsVsObserved`
lineOrientation <- "diagonal"
if(residuals){lineOrientation <- "horizontal"}

I think such options are quite convenient for plotTornado and plotBoxWhisker and might be valuable for users in plotDDIRatio

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe use residualsVsObserved instead of residuals to keep it consistent with dataMapping

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, but with residualsVsObserved = FALSE by default and nullAllowed = FALSE

# Apply log10 transformation to lines because
# plot is log scaled in by default and geom_abline
# requires the log transformed values in input of intercept
self$lines <- lapply(lines, log10)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we may want to apply the log10 directly within the plotXX functions after checking the plotConfiguration scale is included in Scaling$log (obs vs pred and res vs pred might be in linear scale).
Otherwise, users can get unwanted log transformation of their diagonal/horizontal lines.

# Apply log10 transformation to lines because
# plot is log scaled in by default and geom_abline
# requires the log transformed values in input of intercept
self$lines <- lapply(lines, log10)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If possible could you document the scaling in the @param lines and maybe in the pk-ratio vignette

@msevestre msevestre merged commit 82aa7fd into develop Oct 14, 2021
@msevestre msevestre deleted the add-residualVsObserved-ddi-ratio-guest-function-output branch October 14, 2021 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants