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

evaluateModelFit broken? #30

Open
dvrbts opened this issue Nov 25, 2019 · 8 comments
Open

evaluateModelFit broken? #30

dvrbts opened this issue Nov 25, 2019 · 8 comments

Comments

@dvrbts
Copy link

@dvrbts dvrbts commented Nov 25, 2019

I downloaded 3.0-4 earlier this week, and now a function that used to work no longer does.

shosh.hmsc.rand.eval <- evaluateModelFit(hM=shosh.hmsc.rand,
+ predY=shosh.hmsc.rand.preds)
Error in roc.default(response, predictor, auc = TRUE, ...) : 
  No control observation.

This command worked fine recently. If I foolishly repeat the command after the first warning R crashes and I lose eveything from that session.

@oysteiop
Copy link
Collaborator

@oysteiop oysteiop commented Nov 25, 2019

Hi, this function has not changed recently. Could you give us some details about your model structure?

@jarioksa
Copy link
Collaborator

@jarioksa jarioksa commented Nov 25, 2019

Well, the function was changed on Oct 24 (commit cfb442c), and it seems that this changed line may be involved in this error, but I cannot spot any obvious reason for the error in this line. However, the error is triggered in imported code from other packages: Hmsc has no direct call to roc, but this is probably in pROC code imported in evaluateModelFit. However, the last CRAN update of pROC was in July, so that the package has not changed recently.

I cannot reproduce the error. Can you give detailed traceback() after the error or step through the function (with debugonce(evaluateModelFit)) so that we find the ultimate HMSC line that gives the error?

Is the shosh.hmsc.rand saved from a previous session? The old result objects may not be compatible with the new code, and perhaps you should re-calculate the result instead of re-using old results.

@dvrbts
Copy link
Author

@dvrbts dvrbts commented Nov 25, 2019

@jarioksa
Copy link
Collaborator

@jarioksa jarioksa commented Nov 25, 2019

Dave,

The error really seems to be triggered by this Oct 24 commit (cfb442c) which explicitly sets levels = c(0,1) for pROC::auc(): These are not the levels in your data, but you have c(FALSE, TRUE). In general, these are interchangeable in mathematical operations, but the pROC package in one place identifies the control cases as as.character(levels[1]) and "0" and "FALSE" are different names. So it is a peculiarity in the pROC package that was exposed by the change in Hmsc::evaluateModelFit().

I think it is prudent to see how to handle this smoothly within Hmsc. However, an immediate solution is to take care that your data are numeric 0 or 1. Instead of

shoshveg <- shoshveg > 0

do

shoshveg <- ifelse(shoshveg > 0, 1, 0)

or any alternative that turns the logical condition from FALSE and TRUE to numeric 0 and 1. This solved the problem with me when I ran your example.

@gtikhonov
Copy link
Collaborator

@gtikhonov gtikhonov commented Nov 25, 2019

Hi Dave and Jari,

As it was me, who made this commit, I believe that I should provide some explainantion. The previous version of this function was returning result=max(auc,1-auc) due to the somewhat non-obvious behaviour of the used auc / roc function. Apparently, unless you explicitly specify it, the order of your binary values is chosen based on the predicted scores - the one with higher mean predictive score is defined as "positive" and the other one as "negative". I fixed this misbehaviour and, therefore, in my opinion, the new version is not really bugged with respect to this aspect.

I completely agree that with binary data, a boolean type is natural. However, I do not have an clear view of how to generalise the call to auc function, so it would properly handle both 0/1 and FALSE/TRUE options. Any thoughts?

@dvrbts
Copy link
Author

@dvrbts dvrbts commented Nov 25, 2019

@dvrbts
Copy link
Author

@dvrbts dvrbts commented Nov 25, 2019

jarioksa added a commit that referenced this issue Nov 27, 2019
Calculation of AUC in evaluateModelFit assumed data are binary numeric
{0,1}, and logical data {FALSE,TRUE} failed. Now always transforms
data to numeric as assumed later. This is pretty lightweight: in the
example of issue #30 with 150 SUs and 108 species, this took on
average (microbenchmark) 0.6 msec in MacBook Air.
jarioksa added a commit that referenced this issue Nov 28, 2019
Fixes issue #30: evaluateModelFit only handled probit models fitted
with 0/1 data, and failed with equivalent FALSE/TRUE data.
@jarioksa
Copy link
Collaborator

@jarioksa jarioksa commented Nov 28, 2019

I implemented a solution that forces data into {0,1} for AUC in evaluateModelFit (commit 904ad54). An alternative solution would have been to force data into {FALSE,TRUE} and change the AUC call to pROC::auc(..., levels=c(FALSE,TRUE)). In my test this fixed the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.