-
Notifications
You must be signed in to change notification settings - Fork 5
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
60 auc estimation streamline conc scale for estimation add argument #64
60 auc estimation streamline conc scale for estimation add argument #64
Conversation
…-scale, updated the function logic and documentation, added re-parameterized functions with log10 scale x
Merge branch 'dev' of https://github.com/USEPA/CompTox-ToxCast-tcplFit2 into 60-auc-estimation-streamline-conc-scale-for-estimation-add-argument # Conflicts: # R/concRespCore.R
…the plots; Updated the bi-phasic curve section after the poly2 update (more development needed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some minor requested changes. This review is in addition to further testing and review in parallel progress.
logpoly1 <- function(ps, x) { | ||
return(ps[1]*(10^x)) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- poly 2 should be moved up here with poly1
- also, suggest moving the power with the poly2 - i.e. poly models followed by power followed by exponential models
R/get_AUC.R
Outdated
@@ -19,6 +19,9 @@ | |||
#' @param ps Numeric vector (or list) of model parameters for the specified model in `fit_method`. | |||
#' @param return.abs Logical argument, defaults to FALSE. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gracezhihuizhao, can you please provide a little more detail with regard to what this logical argument is meant to do? I assume this returns the absolute value of the AUC. However, current documentation is very basic and uninformative.
vignettes/get_AUC_demo.Rmd
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@madison-feshuk and @brown-jason, this vignette may need to be double checked for any additional explanatory text and/or polishing before merging into 'dev' or release of the next package version.
vignettes/get_AUC_demo.Rmd
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gracezhihuizhao, please check log-scale plots (particularly Fig 1 and Fig 3). Your fix for this may be in another branch , but please check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the plots have the same issue you identified in PR #87. The fix in that PR will also fix the Fig 1 and Fig 3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gracezhihuizhao, in code documentation of the re-param functions to log-scale it may be helpful to have a code comment above the function with the respective 'normal'/'raw' scale parameterized version of the function equation for easy reference.
…ument to post_hit_AUC as well. Added normal parameterization references for log-scale re-parameterized functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looked good to me to merge to dev. I'm glad the text was updated to include poly2 as a biphasic model (at least for now!). I also really like that the default on AUC is currently FALSE since this is somewhat of a beta feature. Nice work!
…stimation-add-argument
Things that are nice to have from the ticket #60 :
apply()
, in my opinion it will make things easier if the function only return a single value.