Closed matdoering closed 5 years ago
Merging #37 into master will decrease coverage by
0.94%
. The diff coverage is0%
.
@@ Coverage Diff @@
## master #37 +/- ##
==========================================
- Coverage 46.41% 45.47% -0.94%
==========================================
Files 33 33
Lines 3342 3411 +69
==========================================
Hits 1551 1551
- Misses 1791 1860 +69
Impacted Files | Coverage Δ | |
---|---|---|
R/multiclass.R | 0% <0%> (ø) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 7922510...ffd5345. Read the comment docs.
Merging #37 into master will decrease coverage by
0.71%
. The diff coverage is0%
.
@@ Coverage Diff @@
## master #37 +/- ##
=========================================
- Coverage 46.41% 45.7% -0.71%
=========================================
Files 33 33
Lines 3342 3394 +52
=========================================
Hits 1551 1551
- Misses 1791 1843 +52
Impacted Files | Coverage Δ | |
---|---|---|
R/multiclass.R | 0% <0%> (ø) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 7922510...1107c09. Read the comment docs.
Thank you Matthias. You are correct there is no roxygen comments, the documentation is directly in the Rd files. Can I please ask you to:
In addition I was wondering: the compute.A.conditional
seems to be calculating an AUC, is that correct?
If so, wouldn't it be possible to use the built-in roc
and auc
functions of pROC instead? This would allow the additional arguments such as percent, partial.auc; and avoid duplicating the AUC calculation.
Otherwise it would be nice if passing those extra arguments would raise an error, to avoid unnecessary confusion, as this would be quite different from the usual behavior of pROC.
Thanks for the review :+1:
Indeed, I think that A(i|j) is an AUC value. Thus, I replaced the conditional calculation from before with the call to pROC::roc
. I've also included examples for typical scenarios with artificial data. The results seem reasonable for all of them.
There are two problems remaining:
It's not possible to output a bona fide multiclass.roc
object because we need to calculate A(i|j) and A(j|i) individually. When you lump the values together in a single roc
object, the auc
function would not be well-defined anymore. Thus, I manually calculate the mean from A(i|j) and A(j|i) and then place it into the auc
slot of the multiclass.roc
list. I guess this is a bit ugly because the resulting object won't work as expected (e.g. calling auc
on the object will fail) since rocs
is not as expected (list of 2-member lists rather than list of roc objects). Unfortunately, I think this cannot be circumvented, except by customizing some other code such as the auc
function.
I have retained my previous code (multiclass.roc.multivariate.custom
) because the results between my original code (custom calculations according to the paper) and the new code (using auc
) are slightly different (run examples with multiclass.roc.multivariate.custom
and multiclass.roc
to compare). I guess that the auc
function normalizes AUCs to be in [0.5,1] and that's causing the difference. The docs say that this can be done (not the default) for partial curves only. Still, I haven't seen an AUC < 0.5 by pROC yet, so I'm a bit puzzled about the interpretation of the AUC here (particularly for the worst-case classifier example where the new function outputs 1 and the old function outputs 0; I guess both interpretations make sense where the first assumes that the output can be flipped and the second one doesn't). Could you investigate this?
I've just fixed the second problem. In the multiclass setting using direction = "auto" (the default) leads to inconsistent decision rules. Now, I am enforcing direction = ">" if direction = "auto" was set to ensure that the results are consistent for multiple calls of roc
across pairs of classes. Now the AUC for the worst-case classifier is also at 0, as expected.
Due to some tight deadlines here I haven't had time to really look into this, and probably won't for the next two weeks I'm afraid. But I will be able to do that after Christmas.
The Travis builds are failing due to some small issues in the help file. If you have a chance to look into it that would be great, otherwise I'll do it once the branch is merged.
Right, I just saw that I forgot to document the direction parameter, which I added. Fixed that.
No problem, I know those kind of deadlines. I wish you a lot of success and a nice christmas!
I merged into a new branch multiclass-hand-till
so that I can have a better look at it before merging into master.
You are correct with point 1), this is not a mutliclass.roc
any longer. I am thinking the best thing to do is to create a new class of object, for instance ht.multiclass.roc
or something like that.
Also as you said the object returned won't be an auc. Currently the auc(multiclass.roc)
method returns a multiclass.auc
object, not an auc
. We should have something similar here.
I checked and the multiclass.auc
is already not really an auc
. For instance it doesn't support stuff like ci
or var
. I will use the opportunity to do some cleanup on that one too. The best thing to do would be to implement those with bootstrap but it's beyond the scope here.
The only other method I could find that consumes amulticlass.roc
is the print
function. Again it will probably easiest to create a new method for ht.multiclass.roc
.
Re point 2) indeed you need to fix the direction. In the bootstrap operations I fix it with the direction obtained on the main ROC curve. I'm not sure if that would make sense here, or if it would even make a difference as you calculate both A(i|j) and A(j|i) anyway.
Quick update. I have started to implement the new class for the new multivariate ROC and AUC (see multiclass-hand-till branch). I called it mv.multiclass.roc and mv.multiclass.auc.
Re. the direction
: I changed a few things so the default behavior for univariate multiclass.roc doesn't change ("auto" makes a lot of sense there). Basically using match.arg
in the parent function and checking if the argument is missing()
. It is now an error to pass "auto" to the new multivariate case. I regret not having been strict enough with invalid argument combinations in the past, so let's not do the mistake again.
I still need to implement:
We need to specify/document:
Finally I'd like to add a few testthat unit tests:
Once all that is done I can merge into master and prepare a release.
I've been implementing and testing the functions above. Pretty happy so far. There are a few changes:
pair_AUCs
any longerThe CI is broken, and has been so for a while for univariate multiclass.roc for a while a believe. See issue #38. I'll have a look at it later and see if I can implement CI for the new mv.multiclass.roc at the same time.
Formulas worked pretty easily. I found a few other issues along the way which were fixed.
I'd still want to look at the behavior upon invalid/incomplete data and make sure it is consistent with the other functions of the package, and then I'll be able to merge into the master branch and prepare for the release.
The code is now available in the main master branch. Thanks for it! I added you as a contributor in the package description. Please let me know if you'd like to be credited differently.
I've implement the approach from Hand & Till for generalizing the AUC to the multi-class setting by adjusting the
multiclass.roc
function. For individual decision values, it still works as before since I'm simply calling the original version of the function. For multivariate decision values, I added a new implementation according to the paper from 2001. Note that for matrix input, the function does not support the additional arguments. I've adjusted the raw R documentation accordingly since I couldn't find any roxygen comments.