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

add confidence interval output to paired DeLong test #95

Merged
merged 16 commits into from Jul 23, 2021

Conversation

wzbillings
Copy link
Contributor

Addresses issue #73 for PAIRED TESTS only using the paper mentioned in the issue discussion.

@wzbillings
Copy link
Contributor Author

I'm not sure what the AppVeyor build failed check means or how to fix it :(

@xrobin
Copy link
Owner

xrobin commented Jul 7, 2021

Thanks for the code, that's really neat!

The AppVeyor CI ensures everything is working properly and that the code can be submitted to CRAN. It runs R CMD check on the package and runs the test suite. At the moment it finds following errors:

1.

Codoc mismatches from documentation object 'roc.test':
roc.test.roc
  Code: function(roc1, roc2, method = c("delong", "bootstrap",
                 "venkatraman", "sensitivity", "specificity"),
                 sensitivity = NULL, specificity = NULL, alternative =
                 c("two.sided", "less", "greater"), paired = NULL,
                 reuse.auc = TRUE, boot.n = 2000, boot.stratified =
                 TRUE, ties.method = "first", progress =
                 getOption("pROCProgress")$name, parallel = FALSE,
                 conf.level = NULL, ...)
  Docs: function(roc1, roc2, method = c("delong", "bootstrap",
                 "venkatraman", "sensitivity", "specificity"),
                 sensitivity = NULL, specificity = NULL, alternative =
                 c("two.sided", "less", "greater"), paired = NULL,
                 reuse.auc = TRUE, boot.n = 2000, boot.stratified =
                 TRUE, ties.method = "first", progress =
                 getOption("pROCProgress")$name, parallel = FALSE, ...)
  Argument names in code not in docs:
    conf.level
  Mismatches in argument names:
    Position: 14 Code: conf.level Docs: ...

This means you should update the documentation (in man/roc.test.Rd) to match the changes in the code.

2.

The error hints at the following line in the test suite:

t1c <- roc.test(aSAH$outcome ~ aSAH$wfns + aSAH$s100b, quiet = TRUE)

The test ensures that no output is produced. However now there's the following output:

ci.level not specified, defaults to 0.95.

Not sure how to fix that cleanly at the moment. Maybe we could default to 0.95?

By the way, I would move the messages to roc.test. Other functions in delong.R don't check/fix their arguments.

@xrobin
Copy link
Owner

xrobin commented Jul 7, 2021

One comment about performance. The delong calculations are now performed twice, in:

		stat <- delong.paired.test(roc1, roc2)
		stat.ci <- ci.delong.paired(roc1, roc2, conf.level)

Although it isn't obvious from the code, and the fact that the function is implemented in C++, the DeLong placements are still very expensive to compute, especially for larger ROC curves. Is it possible to change the code a bit to avoid re-computing everything?

@wzbillings
Copy link
Contributor Author

wzbillings commented Jul 7, 2021

Is it possible to change the code a bit to avoid re-computing everything?

Yes it is possible, and I considered doing this--I think for the best fix, the shared code to be moved into another function which is called by both delong.paired.test() and ci.delong.paired(), maybe delong.paired.calculations() or something similar, that gets called and produces an object which is passed to the test and the CI function. I'll have to pull up the code to see which parts are in common and what needs to be passed to either function. If you have a different suggestion for refactoring or for naming I am happy to do it another way.

If it is refactored in this way I think that delong.test() could also be refactored into one function that calls either delong.paired.calculations() or delong.unpaired.calculations() based on a PAIRED argument. But that is sort of a tangent and for now I will focus on just getting this fix done. (If you have a reference for the unpaired CI though, I'd love to take a look at implementing this in the future.)

Not sure how to fix that cleanly at the moment. Maybe we could default to 0.95?
By the way, I would move the messages to roc.test. Other functions in delong.R don't check/fix their arguments.

I think checking the value of the conf.level argument inside of roc.test() is better anyways, I just didn't think about that before submitting the PR. It's probably easier to just set a default and then run the checks that make sure it's a value that's allowed before passing the argument to the delong functions.

This means you should update the documentation

Easy enough, hopefully :)

@wzbillings
Copy link
Contributor Author

Checks have passed with updated pull request (also refactors the paired DeLong test/CI calculations as described).

@wzbillings
Copy link
Contributor Author

Any updates on whether this can be approved?

@xrobin
Copy link
Owner

xrobin commented Jul 18, 2021

That's great thanks! I've been very busy these days and haven't really had much time to look into this, sorry about that.

For the time being I think this can be merged in a feature branch. To release it / merge into master, I'd like to have the following:

  • At the moment from the doc you can't tell how to retrieve the CI value. Ideally this should be added into the \value{} section in roc.test.Rd.
  • Ideally it would be great if there was a unit test to make sure it keeps working in the future (that would be in tests/testthat/test-roc.test.R)

Last thing, this isn't very fresh in my mind, so I might be wrong (I did that 10 years ago). The DeLong paper describes how to do a paired (correlated) test. But I remember it was reasonably easy to extend to unpaired curves. Do you have a feeling for how easily this could be done? This would keep the interface consistent between paired and unpaired... But I guess that's beyond this pull request.

@wzbillings
Copy link
Contributor Author

I can definitely fix the docs and test. (Sorry, I have no experience with package development so I didn't know this needed to be done.)

Not sure about the unpaired CI extension--the paired CI is based on a pivot quantity which has a standard normal distribution. I think I can emulate the CI calculation for the unpaired test, but it's beyond my knowledge of mathematical statistics to know what distribution the adjusted pivot would have--from the code, I assume a $t$ distribution with the returned degrees of freedom (since the estimate is named $t$ in the code). Can you confirm or deny this?

@xrobin xrobin merged commit 7191a35 into xrobin:master Jul 23, 2021
@xrobin
Copy link
Owner

xrobin commented Jul 23, 2021

That's awesome, thanks a lot!
I'll try to push a release asap. It may take a while.

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.

None yet

2 participants