-
Notifications
You must be signed in to change notification settings - Fork 10
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
New commit for additional statistics #663
Conversation
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.
Thanks @Prerana17 and @0liver0815 please see comments below
R/Design-methods.R
Outdated
) | ||
|
||
# Create list with median MTD and CV. | ||
additional_stats <- list( |
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 is nice I think, however I wonder if we can make this more flexible for the user - e.g. how about accepting a list of functions as part of the simulate()
call and then the user can define median
, cv
functions etc. to apply to target_dose_samples
?
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.
Great proposal. I really like that idea. This will add much flexibility e.g. if someone want to see the mean instead of the median etc.. We just have to evaluate, if it is straight forward to implement.
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.
@danielinteractive @0liver0815 Sure, as the next step, I will try to implement the functions where user can define median, cv, mean etc. functions.
@Prerana17 @ClaraBeckBayer @0liver0815 I am assuming this is the PR moving forward now and not #661 ? or is that not the case? ... please clarify |
After talking to @Prerana17, the other PR is closed and the changes that @ClaraBeckBayer added, that were not part of this PR will be added here, so that this PR is up to date. @danielinteractive I think a review makes only sense for you, if the all checks completed without error. |
Thanks @0liver0815 and @Prerana17 for the clarification :) all good. I did initial review here, please have a look since that could change structure a bit. |
R/Design-methods.R
Outdated
if (is.null(derive)) { | ||
additional_stats <- list() | ||
} else { | ||
additional_stats <- lapply(derive, function(f) f(target_dose_samples)) | ||
} |
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.
If default is an empty list, I think you can get rid of the if statement
if (is.null(derive)) { | |
additional_stats <- list() | |
} else { | |
additional_stats <- lapply(derive, function(f) f(target_dose_samples)) | |
} | |
additional_stats <- lapply(derive, function(f) f(target_dose_samples)) |
R/Design-methods.R
Outdated
if (is.null(derive)) { | ||
additional_stats <- list() | ||
} else { | ||
additional_stats <- lapply(derive, function(f) f(target_dose_samples)) | ||
} |
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.
I think it is the same here. lapply works with an empty list.
if (is.null(derive)) { | |
additional_stats <- list() | |
} else { | |
additional_stats <- lapply(derive, function(f) f(target_dose_samples)) | |
} | |
additional_stats <- lapply(derive, function(f) f(target_dose_samples)) |
R/Design-methods.R
Outdated
# Create a function for additional statistical summary. | ||
if (is.null(derive)) { | ||
additional_stats <- list() | ||
} else { | ||
additional_stats <- lapply(derive, function(f) f(target_dose_samples)) | ||
} |
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.
Same here.
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.
I think you need to add derive = list()
to the function as done in line 212 above.
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.
thanks @Prerana17 , nice work! please see comments below, can you also add a NEWS
entry?
median_mtd = median(target_dose_samples), | ||
cv_mtd = mad(target_dose_samples) / median(target_dose_samples) | ||
) | ||
# Create a function for additional statistical summary. |
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.
same here
R/Design-methods.R
Outdated
@@ -209,7 +212,7 @@ setMethod("simulate", | |||
truth, args = NULL, firstSeparate = FALSE, | |||
mcmcOptions = McmcOptions(), | |||
parallel = FALSE, nCores = | |||
min(parallel::detectCores(), 5), | |||
min(parallel::detectCores(), 5), derive = list(), | |||
...) { | |||
nsim <- safeInteger(nsim) |
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.
please assert derive
properties here (list, named, unique names, elements are functions) using checkmate
package functions
|
||
# Create a function for additional statistical summary. | ||
|
||
additional_stats <- lapply(derive, function(f) f(target_dose_samples)) |
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.
additional_stats <- lapply(derive, function(f) f(target_dose_samples)) | |
additional_stats <- lapply(derive, f, target_dose_samples) |
should work too I guess?
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.
It's throwing error. So I kept same.
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.
yep makes sense actually
R/Simulations-class.R
Outdated
o$check( | ||
identical(length(object@additional_stats), nSims), | ||
"additional_stats must have same length as the number of simulations" | ||
) |
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.
also check that it has names?
|
||
cat( | ||
"Results of Additional Statistical Calculation : \n", | ||
paste(names(summary_stat_op), ":", round(summary_stat_op), "\n") |
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.
ok as a first version, just wondering if we should follow up in another PR to make this printing more flexible
Code Coverage Summary
Diff against main
Results for commit: 9cdeabe Minimum allowed coverage is ♻️ This comment has been updated with latest results |
…into 656_additional_stat
* Refactor setSeed and getResultList (#676) Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * 675: Add `DesignGrouped` (#677) Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * Fixes first cohort logic for grouped design simulation (#684) * Remove references to is.bool and is.scalar (#686) * Remove references to is.bool and is.scalar * [skip actions] Restyle files * Responding to review comments * Removing lintr moan * Updating documentation --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * Remove plot.gtable and print.gtable (#697) * Remove plot.gtable and print.gtable * Fix _pkgdown.yaml * 690 remove safeinteger and iswholenumber 1 (#693) * Remove safeInteger and is.wholenumber * Fixing false test failures * [skip actions] Restyle files --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * Remove noOverlap (#695) * remove %~% (#689) * remove %~% * Fixing NAMESPACE * 699 remove multiplot (#700) * Remove multiplot * Correct typo * Correcting typo in argument to assert_integerish * New commit for additional statistics (#663) * New commit for additional statistics * [skip actions] Restyle files * Update: additional statstics added to other designs bug correction suggetions by Clara * bug corrected * Update: Additional_stats * update: additional_stats * updaze: error_corrections (additional_stats) * update: additional_statistics * Dynamic: Additional_statistical_summary * Additional statistical summary function update * additional statistical summary changes * Update: Additional Statistical Summary * small change: additional stat summary * Suggested changes: additional_statistics and validation * [skip actions] Restyle files * Simulation refectoring * Simulation small update * [skip actions] Restyle files * small update * incorrectly named file:delete * removing documentation errors * update yaml file --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Clara <clara.beck@bayer.com> Co-authored-by: Oliver Boix <95433070+0liver0815@users.noreply.github.com> Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com> * Refactor myBarplot (#703) * Refactor myBarplot * [skip actions] Restyle files * Responding to lintr complaints * Rsponding to review comments --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> --------- Co-authored-by: Oliver Boix <95433070+0liver0815@users.noreply.github.com> Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Prerana Chandratre <82475780+Prerana17@users.noreply.github.com> Co-authored-by: Clara <clara.beck@bayer.com>
closes ##656