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

Update Trial Planning Vignette #71

Merged
merged 9 commits into from
Oct 2, 2023
Merged

Update Trial Planning Vignette #71

merged 9 commits into from
Oct 2, 2023

Conversation

holgstr
Copy link
Contributor

@holgstr holgstr commented Sep 28, 2023

Refactoring trial planning vignette:

  • a few sentences are rephrased to match wording of new functions
  • changes name of powerEmp to empSignificant

@holgstr holgstr added the enhancement New feature or request label Sep 28, 2023
@holgstr holgstr self-assigned this Sep 28, 2023
@holgstr
Copy link
Contributor Author

holgstr commented Sep 28, 2023

Dear @danielinteractive, I decided to reproduce scenario 2 from the paper for the vignette. However, I cannot reproduce the required OS sample size according to Schoenfeld (732 in the vignette, 708 in the paper). See also the sample code in the linked issue opened by Kaspar.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 28, 2023

badge

Code Coverage Summary

Filename                     Stmts    Miss  Cover    Missing
-------------------------  -------  ------  -------  ---------
R/assertions.R                  15       0  100.00%
R/empSignificant.R              59       0  100.00%
R/eventTracking.R               88       2  97.73%   48, 244
R/getClinicalTrials.R           64       0  100.00%
R/getSimulatedData.R           107       0  100.00%
R/getWaitTimeSum.R              29       2  93.10%   45, 48
R/hazardFunctions.R             26       0  100.00%
R/piecewiseDistribution.R       32       0  100.00%
R/piecewiseHazards.R            18       0  100.00%
R/survivalFunctions.R           96       0  100.00%
R/transitionParameters.R        42       0  100.00%
TOTAL                          576       4  99.31%

Diff against main

Filename                     Stmts    Miss  Cover
-------------------------  -------  ------  --------
R/empSignificant.R             +59       0  +100.00%
R/eventTracking.R               +8       0  +0.23%
R/getClinicalTrials.R           +6       0  +100.00%
R/getSimulatedData.R            +5       0  +100.00%
R/hazardFunctions.R            +26       0  +100.00%
R/piecewiseDistribution.R       -1       0  +100.00%
R/piecewiseHazards.R            +1       0  +100.00%
R/survivalFunctions.R          +96       0  +100.00%
TOTAL                         +200       0  +0.37%

Results for commit: 0b049b0

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@danielinteractive
Copy link
Collaborator

Interesting thanks @holgstr , so is there now new content in the vignette? Or just refactored? I would go first for pure refactoring and then in separate PR add new content.

@holgstr
Copy link
Contributor Author

holgstr commented Sep 28, 2023

Hm, I guess I might have misunderstood what „refactoring“ means? 😄 I have substituted the R code with the newly added functions, renamed the powerEmp function to empSignificance, and along the way rephrased some sentences in the vignette to match the wording of the package/functions better. The only truly new content is the small chunk in the vignette where sample sizes are calculated according to Schoenfeld via rpact. I now see you may prefer me to break up the PR into two smaller ones? Should I do this now or rather keep it in mind going forward?

@danielinteractive
Copy link
Collaborator

I would say the problematic part of it is new where you cannot reproduce the paper let's split this into separate PR.

@danielinteractive
Copy link
Collaborator

Sorry about the confusion Kaspars issue refers to the new addition, let's keep this PR to the refactoring I.e. using the new functions you added etc

@holgstr
Copy link
Contributor Author

holgstr commented Sep 29, 2023

I have left out the addition of computing the sample sizes via Schoenfeld, so this PR now includes only refactoring + renaming powerEmp to empSignificant.

@github-actions
Copy link
Contributor

Unit Tests Summary

    1 files    14 suites   2m 26s ⏱️
  57 tests   57 ✔️ 0 💤 0
138 runs  138 ✔️ 0 💤 0

Results for commit 0b6096c.

@holgstr holgstr merged commit 51d9d5f into main Oct 2, 2023
@holgstr holgstr deleted the vignette-new branch October 2, 2023 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants