fix false-> true

[audreyyeoCH]

closes #146

[github-actions]

Code Coverage Summary

Filename                 Stmts    Miss  Cover    Missing
---------------------  -------  ------  -------  -----------
R/betadiff.R                78       0  100.00%
R/boundsPostprob.R          44       0  100.00%
R/boundsPredprob.R          56       1  98.21%   83
R/dbetabinom.R              99       3  96.97%   32, 62, 154
R/oc2.R                    162     162  0.00%    93-326
R/oc3.R                    146     146  0.00%    91-308
R/ocPostprob.R             117       0  100.00%
R/ocPostprobDist.R         122       0  100.00%
R/ocPredprob.R             205       0  100.00%
R/ocPredprobDist.R         257       5  98.05%   363-367
R/ocRctPostprobDist.R      174       0  100.00%
R/ocRctPredprobDist.R      302       0  100.00%
R/plotBeta.R                97       9  90.72%   124-132
R/plotBounds.R              83      83  0.00%    42-130
R/plotDecision.R            73       0  100.00%
R/plotOc.R                  55       2  96.36%   99-100
R/postprob.R                34       1  97.06%   113
R/postprobDist.R            77       1  98.70%   204
R/predprob.R                36       0  100.00%
R/predprobDist.R           140       1  99.29%   269
R/runShinyPhase1b.R          4       4  0.00%    8-13
R/sumBetaDiff.R             80      15  81.25%   102-120
R/sumTable.R                36       0  100.00%
TOTAL                     2477     433  82.52%

Diff against main

Filename      Stmts    Miss  Cover
----------  -------  ------  --------
TOTAL             0       0  +100.00%

Results for commit: 3d83aec

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

[danielinteractive]

As usual when there is a bug fix can you please add a test that first reproduces the bug and then is fixed with this change?

[github-actions]

Unit Tests Summary

    1 files     20 suites   9m 44s ⏱️
  141 tests   138 ✅ 0 💤 2 ❌ 1 🔥
2 581 runs  2 577 ✅ 0 💤 3 ❌ 1 🔥

For more details on these failures and errors, see this check.

Results for commit f25bf7a.

♻️ This comment has been updated with latest results.

[github-actions]

Unit Test Performance Difference

Test Suite	$Status$	Time on main	$±Time$	$±Tests$	$±Skipped$	$±Failures$	$±Errors$
ocPredprob	💚	$210.46$	$-7.84$	$+9$	$0$	$0$	$0$
ocPredprobDist	💚	$96.94$	$-3.24$	$0$	$0$	$0$	$0$
plotOc	💚	$214.92$	$-5.41$	$0$	$0$	$0$	$0$
predprobDist	💚	$26.26$	$-10.82$	$-1192$	$0$	$0$	$+1$

Additional test case details

Test Suite	$Status$	Time on main	$±Time$	Test Case
ocPredprob	👶		$+0.43$	Go_decision_for_all_interims_when_criteria_met_for_decision_1
ocPredprob	💚	$207.62$	$-8.24$	ocPredprob_correctly_shows_maximum_sample_size_when_no_decision_reached
ocPredprobDist	💚	$62.42$	$-2.18$	ocPredprobDist_gives_higher_PrFutility_with_more_futility_looks
plotOc	💚	$57.16$	$-1.45$	h_get_dataframe_oc_gives_correct_results_for_ocPredprobDist_when_relativeDelta_FALSE
plotOc	💚	$105.52$	$-2.87$	plotOc_gives_expected_results_for_ocPredprobDist_with_different_relativeDelta_status
predprobDist	💚	$2.29$	$-2.06$	h_predprobdist_gives_correct_list
predprobDist	💔	$6.59$	$+2.30$	h_predprobdist_gives_higher_predictive_probability_when_thetaT_is_lower
predprobDist	💚	$11.91$	$-11.89$	predprobDist_gives_higher_predictive_probability_when_thetaT_is_lower_in_a_single_arm_trial

Results for commit 2879ab0

♻️ This comment has been updated with latest results.

[danielinteractive]

Thanks @audreyyeoCH !

[danielinteractive]

Good to go from my side already before, but you first have to fix the check errors

[audreyyeoCH]

Good to go from my side already before, but you first have to fix the check errors

thanks @danielinteractive, did you see the comment from above ? I wasn't able to get a link

[audreyyeoCH]

Hi @danielinteractive , in h_getBetamixPost, I added a warning a few issues ago, for input weights that don't add up to 1. The test for anything that relies on this helper therefore needs to handle the repeated warnings and I found this solution.

This solution may be measured to the risk of the wrong user weight input. If we believe so, we can incorporate the multiple warnings in the test, to ensure it gives the correct number of the same warnings.

This solution may be seen as an overkill - if we remove the warning of "Weights have been corrected", and someone decides to add it later on, it will cause a chain of warnings again in all the call functions that have h_getBetamixPost as their helper function.

Thought I'd check in with your views on the what the risk vs benefit is before I add it to the other tests. Thanks!

[danielinteractive]

Thanks @audreyyeoCH , good point. I think for the tests it might be ok, but I am more concerned then about the user experience: Is this helper function called inside simulation loops somewhere e.g. and might trigger as many warnings as there are simulation iterations?

If yes, I think we need to do something about it.

However, for the topic of this PR this seems tangential at best, so it would be better to handle this in a separate issue / PR if possible. That is, can we not leave this particular test as it was before in this PR?

[audreyyeoCH]

Thanks for looking into it @danielinteractive.

Is this helper function called inside simulation loops somewhere e.g. and might trigger as many warnings as there are simulation iterations?

edit: the number of warnings is not the number of simulations. It is the number of arms + number of posteriors calculated * number of arms. In addition, the number of warnings is capped at 50, with this warning

There were 50 or more warnings (use warnings() to see the first 50)

The number of posteriors in this scenario is:

for active arm : 17 patients left to recruit, therefore 18 possible results (0....17 responders)
for control arm: 10 patients left to recruit, therefore 11 possible results (0...11 responders).

This is an array of 18 x 11 elements, making it a total of 198 possible combination of results from each arm. We have two warnings per posterior because there are 2 arms, making it a sum of 396 warnings.

plus the posterior from calculating each arm. This is 2.

In total it is 396 + 2 warnings = 398 warnings. The number of same warnings is 398.

I do not believe it affects the user experience, because of the one line of warning above. Thus I feel we can go either way to carry on with ensuring all tests capture warnings or completely remove the warning from h_getBetamixPost, such that the user carries their own risk of mis-specifying their weights since ... $params will show what weights were used. What do you think ?

However, for the topic of this PR this seems tangential at best, so it would be better to handle this in a separate issue / PR if possible. That is, can we not leave this particular test as it was before in this PR?

I agree it seems tangential, but it is causing the checks not to pass. Is there another way around this that I am not seeing ?

[danielinteractive]

OK I ran checks on the main branch and they are failing because of the warnings (and few other things):
https://github.com/Genentech/phase1b/actions/runs/20737971973/job/59539040271

I would recommend to first make a separate issue/PR to fix this, get the main branch into a clean state, and then continue with this issue/PR.

As an idea for the fix, I think so many warnings are just too many. Either we move the warning up to the user level function such that it is only given once per user call. Or we just don't warn.