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

#8: remove yardstick #9

Merged
merged 5 commits into from
Feb 17, 2025
Merged

#8: remove yardstick #9

merged 5 commits into from
Feb 17, 2025

Conversation

slamao
Copy link
Collaborator

@slamao slamao commented Feb 14, 2025

Close #8

Copy link
Contributor

github-actions bot commented Feb 14, 2025

badge

Code Coverage Summary

Filename                  Stmts    Miss  Cover    Missing
----------------------  -------  ------  -------  ------------------------------------------------------------------
R/calibrationProfile.R      147       0  100.00%
R/getEstimates.R            370      37  90.00%   213, 359-364, 405-409, 412-419, 422-429, 439-441, 450-452, 456-458
R/locf.R                     43       0  100.00%
R/PV.R                      134       0  100.00%
R/riskProfile.R             210       0  100.00%
R/sensSpec.R                 66       0  100.00%
R/utils.R                   164       0  100.00%
TOTAL                      1134      37  96.74%

Diff against main

Filename        Stmts    Miss  Cover
------------  -------  ------  --------
R/PV.R            -12       0  +100.00%
R/sensSpec.R       +2       0  +100.00%
R/utils.R         +20       0  +100.00%
TOTAL             +10       0  +0.03%

Results for commit: 4be0e01

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

Copy link
Contributor

github-actions bot commented Feb 14, 2025

Unit Tests Summary

  1 files   7 suites   6s ⏱️
 36 tests 23 ✅ 13 💤 0 ❌
114 runs  89 ✅ 25 💤 0 ❌

Results for commit 4be0e01.

♻️ This comment has been updated with latest results.

@dhibar
Copy link

dhibar commented Feb 14, 2025

@slamao With this update I think you'd need to make the same changes to the nonParametricPV function like you did in PR #7 i.e. removing [1:(length(score) - 1)] and update the prevalence padding in threshold.data.

Maybe you already plan to do this as part of the merging (sorry I'm not very good at Github)

Otherwise I tested out the changes to the ppv/npv functions and it all looks reasonable!

@slamao
Copy link
Collaborator Author

slamao commented Feb 14, 2025

@slamao With this update I think you'd need to make the same changes to the nonParametricPV function like you did in PR #7 i.e. removing [1:(length(score) - 1)] and update the prevalence padding in threshold.data.

Maybe you already plan to do this as part of the merging (sorry I'm not very good at Github)

Otherwise I tested out the changes to the ppv/npv functions and it all looks reasonable!

Thanks for testing it out :)
And yes, I firstly need to merge the updated main branch into this branch (see above) and merge this PR only afterwards.

@slamao slamao merged commit 43f542e into main Feb 17, 2025
11 checks passed
@slamao slamao deleted the 8_yardstick_speed_up branch February 17, 2025 08:13
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.

Speed up nonParametricPV
2 participants