search

ddsjoberg / dcurves

Decision Curve Analysis
http://www.danieldsjoberg.com/dcurves/
Other
37 stars 14 forks source link

Allow including 0% threshold #3

Closed ck37 closed 3 years ago

ck37 commented 3 years ago

Hello,

Thanks for the very useful package. I'm using it for some clinical risk modeling and two changes were helpful for me: 1) allow removing the default all or none strategies, and 2) don't actively exclude the 0% threshold. Here is a quick PR for these two edits.

Cheers, Chris

ck37 commented 3 years ago

Hi @ddsjoberg, any reaction to this?

Thanks, Chris

ddsjoberg commented 3 years ago

hello @ck37 ! Thank you for following up!

I think this PR was lost in a frenzy of github notifications. I"ll take a look in the next couple of weeks!

FYI, while I am confident in the results you've obtained using the package, the package is currently undergoing a thorough code review prior to its submission to CRAN.

ddsjoberg commented 3 years ago

@ck37 at first glance, I think I may prefer to include the options of which strategies to include in the plot method. The treat all strategy is required to calculate the net interventions. Anyway, i'll think on it!

ck37 commented 3 years ago

Ah excellent point, and for my use case I only wanted to remove the "none" strategy from the plots.

ddsjoberg commented 3 years ago

I've worked with DCA for years, and I've never seen or heard of a request to not plot the treat all or treat none line on the figure. Can you give some background on why the treat none line should be omitted from your figure? Thanks! @ck37

ck37 commented 3 years ago

Well, it's just a flat line at zero, so plotting "treat none" is not really informative or necessary. In my case I primarily wanted a shorter legend because I had 7 models and "treat all" to include. Here is an example paper that also doesn't plot "treat none": https://www.ahajournals.org/doi/full/10.1161/JAHA.120.020082

ck37 commented 3 years ago

Ok simplifying this PR to just allow calculating and plotting the 0% threshold

ddsjoberg commented 3 years ago

@ck37 thanks for the update! I agree it's better to address these two questions separately.

Can you confirm a couple of things? I'm away from a computer until Wednesday ⛱️🌊🏖️, so I can't check at the moment.

  1. At a threshold of 0, does the treat none line have a calculated net benefits of 1 or 0? We may need to code to handle that case.
  2. The default threshold values should also be updated to include 0. Thoughts?
ck37 commented 3 years ago

Excellent points. Yep treat none was switching to a standardized net benefit of 1 at the 0% threshold, so I added a line to fix that. Updated the default thresholds to include 0 as well.

And no rush at all on this, it's a good time to take a break.

ddsjoberg commented 3 years ago

@ck37 oy! Apologies, I was playing around, and I accidentally pushed my changes here....I'll attempt a revert soon!

ck37 commented 3 years ago

@ddsjoberg no worries at all, please do whatever is easiest/best for you. I realize that my edits may also not be the cleanest (in part due to my own laziness) so no pressure on my end to use.

ddsjoberg commented 3 years ago

the edits you made are great! the only bit remaining to figure out is how to treat binary decision models, e.g. some models/markers are binary like family history of cancer. These models are passed as 0/1, but the way we have the thresholds now that include 0, the NB for those models shoots to 1 (for standardaized NB) or the prevalence. So the issue wasn't isolated to the treat none strategy.

I am thinking the best/easiest move may be to just replace thresholds of 0 with 10e-10?

ddsjoberg commented 3 years ago

@ck37 hey hey!

I think this should be ready to go. Can you take a quick pass at the changes I made before we merge?

ck37 commented 3 years ago

@ddsjoberg looks great to me. I also reran it on my data and it works like a charm.

ddsjoberg commented 3 years ago

great, thank you so much! I love that the figures start at zero now (or t least look like they do!)