Open manchen07 opened 1 year ago
I hope you haven't started to review this pull request. I've just edited the code for model estimates tables and Bayesian plots. They seem to work. I will work on the code chunks later this week. Feel free to start the review any time. I won't edit more :)
Attention: 5 lines
in your changes are missing coverage. Please review.
Comparison is base (
78447c2
) 77.86% compared to head (476c8ab
) 81.22%.:exclamation: Current head 476c8ab differs from pull request most recent head 1148848. Consider uploading reports for the commit 1148848 to get more accurate results
Files | Patch % | Lines |
---|---|---|
R/calc_BCSMD.R | 98.15% | 5 Missing :warning: |
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
I added some unit tests for calc_consts()
. Do you have any thoughts on how to test g_mlm_Bayes()
? I'm stumped.
I added some unit tests for
calc_consts()
. Do you have any thoughts on how to testg_mlm_Bayes()
? I'm stumped.
Thanks for adding the unit tests for calc_consts()
.
Regarding the g_mlm_Bayes()
, to calculate the denominator of BCSMD, we have now followed the tradition by calculating the r_const
first, then multiplying r_const
by all random effects components from the posterior draws. Previously I just grabbed the relevant random effects variance covariance from the posterior distribution. What do you think if we compare g_mlm_Bayes()
to the previous approach?
Regarding the
g_mlm_Bayes()
, to calculate the denominator of BCSMD, we have now followed the tradition by calculating ther_const
first, then multiplyingr_const
by all random effects components from the posterior draws. Previously I just grabbed the relevant random effects variance covariance from the posterior distribution. What do you think if we compareg_mlm_Bayes()
to the previous approach?
Sure, that seems like it would be a good test, and it would also work as an additional check on calc_consts()
(I think?).
Regarding the
g_mlm_Bayes()
, to calculate the denominator of BCSMD, we have now followed the tradition by calculating ther_const
first, then multiplyingr_const
by all random effects components from the posterior draws. Previously I just grabbed the relevant random effects variance covariance from the posterior distribution. What do you think if we compareg_mlm_Bayes()
to the previous approach?Sure, that seems like it would be a good test, and it would also work as an additional check on
calc_consts()
(I think?).
Yes, that's right! I'll send an update once I'm done with adding the tests.
calc_BCSMD()
. Remaining outstanding problems:
summary = TRUE
in the replication syntax in order to have a nice table output. The problem is that the output of calc_BCSMD()
if using summary = TRUE
won't have the model fit, which will affect the graphing function. g_mlm_Bayes()
and calc_consts()
functions that we created in the package that are required for the shiny app to run. I guess one possible solution is to export these two functions in the package, just as what we did for default_times()
.Design question: Do we want to have separate s3 methods for generating a detailed summary table (like we presented in the 3-level BC-SMD paper) and for generating a one-line summary of the effect size estimate?
Hi James,
I think the R-CMD-check and test-coverage failed on most of these systems because brms is not running. I tried to follow what brms package developers did in the R-CMD-check.yaml and test-coverage.yaml, but still could not figure out how to get brms run on these platforms...Maybe we can discuss more tomorrow. Our unit tests passed on my computer and on the devel version of ubuntu...
Man
Based on our discussion yesterday, I've added a
calc_consts()
function to the package to simply the effect size calculation code. Basically, for this current push, the Bayesian estimation is added to the package and the app. However, there are remaining issues for the app.I added a RUN button for RML and Bayesian. I like it because model will only run when the button is clicked instead of keep running and updating every time we change anything. One outstanding issue is that we need to find a better place for the center slider because the center point will need to be specified before running the model. One solution is to move the center slider above the RUN button.
I added a progress indicator. It is shown at the bottom right of the screen. It just flashes for one second for RML but will show longer for Bayesian estimation. Not sure if there is a better way to indicate the progress of model fitting.
For the Bayesian estimation
I will work on the Bayesian model estimates tables, Bayesian plots, and the syntax code chunks for the rest of this week. Feel free to edit the code and push your edits. I won't push any more commits until you finish reviewing and editing. Let me know if you have any questions.