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

96 bounds predprob #105

Open
wants to merge 64 commits into
base: main
Choose a base branch
from
Open

96 bounds predprob #105

wants to merge 64 commits into from

Conversation

audreyyeoCH
Copy link
Collaborator

closes #96

Copy link
Contributor

github-actions bot commented May 26, 2024

badge

Code Coverage Summary

Filename                 Stmts    Miss  Cover    Missing
---------------------  -------  ------  -------  ----------------------------------
R/betadiff.R                59       0  100.00%
R/boundsPostprob.R          44       0  100.00%
R/boundsPredprob.R          53       1  98.11%   77
R/dbetabinom.R              81       3  96.30%   32, 62, 136
R/oc2.R                    162     162  0.00%    93-326
R/oc3.R                    146     146  0.00%    91-308
R/ocPostprob.R              99       4  95.96%   239-242
R/ocPostprobDist.R         109       0  100.00%
R/ocPredprob.R             182       4  97.80%   285-288
R/ocPredprobDist.R         235       3  98.72%   346-348
R/ocRctPostprobDist.R      154       4  97.40%   236-239
R/ocRctPredprobDist.R      283      55  80.57%   207-222, 225-240, 373-376, 403-421
R/plotBeta.R               103      90  12.62%   60-167
R/plotBounds.R              52      52  0.00%    33-90
R/plotDecision.R            79      79  0.00%    16-136
R/plotOc.R                  15      15  0.00%    16-36
R/postprob.R                34       1  97.06%   106
R/postprobDist.R            77       1  98.70%   204
R/predprob.R                24       0  100.00%
R/predprobDist.R           140       1  99.29%   269
R/runShinyPhase1b.R          4       4  0.00%    8-13
R/sumbetadiff.R             63      63  0.00%    24-101
R/sumTable.R                20      20  0.00%    25-48
TOTAL                     2218     708  68.08%

Diff against main

Filename              Stmts    Miss  Cover
------------------  -------  ------  -------
R/boundsPredprob.R      +25     -27  +98.11%
R/predprobDist.R         -1       0  -0.01%
TOTAL                   +24     -27  +1.58%

Results for commit: c661bdf

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

Copy link
Contributor

github-actions bot commented May 26, 2024

Unit Tests Summary

    1 files     15 suites   1m 50s ⏱️
  119 tests   119 ✅ 0 💤 0 ❌
1 266 runs  1 266 ✅ 0 💤 0 ❌

Results for commit 258ae08.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented May 26, 2024

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
boundsPredprob 👶 $+0.18$ $+18$ $0$ $0$ $0$
Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
boundsPredprob 👶 $+0.13$ boundsPredprob_gives_correct_result_and_when_default_weight_is_not_assigned
boundsPredprob 👶 $+0.05$ boundsPredprob_with_Beta_Mixture_Priors_give_correct_results

Results for commit 5aed66e

♻️ This comment has been updated with latest results.

@audreyyeoCH
Copy link
Collaborator Author

audreyyeoCH commented Oct 7, 2024

I found some test cases failing and am working on it before I feel confident to request PR. This test case seems to be unrelated to the upper boundary of density in predprob() being 1 and not 1 + Machine$double.eps. Still working on it...

@audreyyeoCH audreyyeoCH marked this pull request as draft October 7, 2024 21:37
@audreyyeoCH
Copy link
Collaborator Author

I found some test cases failing and am working on it before I feel confident to request PR. This test case seems to be unrelated to the upper boundary of density in predprob() being 1 and not 1 + Machine$double.eps. Still working on it...

I solved it! The test case passed whenpredprob() used to check these results.

@audreyyeoCH
Copy link
Collaborator Author

Hm I see. I think only some names change in the output of boundsPredprob right? In that case could just have a temporary object assigned the result of it, and then rename and pass to plotBounds. I would recommend keeping that example such that later it will be easy to compare when you refactor plotBounds. So a little bit extra code now is worth it.

ahh true that - thanks! I added it back now... and plotBounds(boundsPOSTprob)) 😆 , thanks for your review!

Copy link
Collaborator

@danielinteractive danielinteractive left a comment

Choose a reason for hiding this comment

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

Thanks @audreyyeoCH , nice work, please see comments below

R/boundsPredprob.R Outdated Show resolved Hide resolved
R/boundsPredprob.R Outdated Show resolved Hide resolved
R/boundsPredprob.R Outdated Show resolved Hide resolved
R/boundsPredprob.R Show resolved Hide resolved
R/boundsPredprob.R Outdated Show resolved Hide resolved
R/boundsPredprob.R Outdated Show resolved Hide resolved
examples/boundsPredprob.R Outdated Show resolved Hide resolved
examples/ocPredprobDist.R Outdated Show resolved Hide resolved
@@ -1,74 +1,86 @@
#' Decision cutpoints for boundary (based on predictive probability)
#' Decision cutpoints for boundary (based on predictive probability) for Decision 1 rule.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Has this been added already?

examples/boundsPredprob.R Outdated Show resolved Hide resolved
audreyyeoCH and others added 2 commits January 9, 2025 10:49
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
@audreyyeoCH
Copy link
Collaborator Author

audreyyeoCH commented Jan 13, 2025

ok so I had some debugging to do...

the t(parE) seems to be duplicating the efforts of helper functions postprob() and predprob() so I removed it. Which created downward problem with ìfmissing(weight)`, so I fixed that too.

the tests are a little more improved too. The first test tests if weights are there, the second builds on what if the weights are there and there are mixed priors.

lastly, due to these changes, I wanted to

  1. make it easy for the user to specify alpha and beta parameters in beta mixture priors
  2. not get the answer wrong

therefore, I improved the documentation of inherited parameters of parE in postprob() and predprob() .... which I learned, don't inherit parameters from each other ... but boundsPredprob should be fine and ready for review soon.

Copy link
Collaborator

@danielinteractive danielinteractive left a comment

Choose a reason for hiding this comment

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

Thanks @audreyyeoCH , almost there!
Btw it would be good to soon come back to a state where only 1 PR is in the works, and the PR is as small as possible. That way, it will be quicker to finish and get to a state where all tests pass etc. again.

R/boundsPredprob.R Outdated Show resolved Hide resolved
R/postprob.R Outdated Show resolved Hide resolved
@audreyyeoCH
Copy link
Collaborator Author

audreyyeoCH commented Jan 14, 2025

Thanks @audreyyeoCH , almost there! Btw it would be good to soon come back to a state where only 1 PR is in the works, and the PR is as small as possible. That way, it will be quicker to finish and get to a state where all tests pass etc. again.

Yes for sure! I can focus on this one first since it's the oldest

@@ -52,7 +52,7 @@ boundsPredprob <- function(looks, Nmax = max(looks), p0, tT, phiL, phiU, parE =
if (missing(weights)) {
weights <- rep(1, nrow(t(parE)))
}
assert_numeric(weights, min.len = 0, len = nrow(par), finite = TRUE)
assert_numeric(weights, min.len = 0, len = nrow(parE), finite = TRUE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If parE is passed as a vector then this would fail, I guess we also need t(parE), as in line 53?

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.

96_boundsPredprob.R
2 participants