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

#6: addressing shift in PPV by one row #7

Merged
merged 1 commit into from
Feb 14, 2025
Merged

#6: addressing shift in PPV by one row #7

merged 1 commit into from
Feb 14, 2025

Conversation

slamao
Copy link
Collaborator

@slamao slamao commented Feb 14, 2025

Address #6.

Since it was chosen by design to assess score > x for each x in score, I don't think we can naturally end up with a PPV at prevalence.

Other option would be to inflate the resulting dataset manually for this boundary value, but than we would need to artificially add one more score and one more percentile to match the number of rows in the resulting data frame.

@slamao slamao marked this pull request as ready for review February 14, 2025 15:01
Copy link
Contributor

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                      146       0  100.00%
R/riskProfile.R             210       0  100.00%
R/sensSpec.R                 64       0  100.00%
R/utils.R                   144       0  100.00%
TOTAL                      1124      37  96.71%

Diff against main

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

Results for commit: 2b0ece7

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

Copy link
Contributor

Unit Tests Summary

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

Results for commit 2b0ece7.

@dhibar
Copy link

dhibar commented Feb 14, 2025

@slamao just tested this out and it all looks great!

One additional thing that I noticed is that there are duplicate rows created in the threshold.data output of the nonParametricPV function e.g. when multiple patients have the exact same score. You can see it if you duplicate one of the values in the toy example that I gave in #6. Ultimately I don't think it is a huge issue because in the plots the same point will just be plotted (i.e. not affecting visual interpretation) and if the user selects a specific value from the table of outputs from riskProfiles they can always just de-duplicate the outputs themselves. I didn't fully check things out though so maybe you already have code to handle this downstream and I am just missing it :)

Nice to hear from you btw!

@slamao
Copy link
Collaborator Author

slamao commented Feb 14, 2025

Thanks for confirming so quickly Derrek!

Regarding the duplication. It is true that this is not checked in the package. I think though that if you pass duplicated values then you should also get duplicated outcomes (for consistency reasons). I would suggest that this is something users could handle themselves already in the input data. Please let me know if you think about it differently though.

Nice to hear from you as well, and happy to hear that somebody is actually using the package 😄 👍

@slamao slamao merged commit 306ba82 into main Feb 14, 2025
21 checks passed
@slamao slamao deleted the 6_ppv_shift branch February 14, 2025 17:10
@dhibar dhibar mentioned this pull request Feb 14, 2025
@dhibar
Copy link

dhibar commented Feb 14, 2025

Ha! I'm an avid user now, thanks for building it!

That totally makes sense about consistency. Downstream users (i.e. me) can just deal with duplicates if they need to.

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.

2 participants