search

xrobin / pROC

Display and analyze ROC curves in R and S+
https://cran.r-project.org/web/packages/pROC/
GNU General Public License v3.0
121 stars 31 forks source link

NAMESPACE / S3 #4

Closed berndbischl closed 9 years ago

berndbischl commented 9 years ago

Hi,

we use multiclass.roc in mlr here:

https://github.com/berndbischl/mlr

It seems to be that auc.roc etc are S3 methods in your package. But you do not mark them as such in your NAMESPACE, which is probably incorrect.

In mlr this triggers now a bug, where we requireNamespace("pROC"), then call multiclass.roc, then this does not find auc.roc, although that function lives in the same package.

Could this please be fixed?

xrobin commented 9 years ago

Hi,

Thanks for the report. Indeed there is quite some mess in pROC's NAMESPACE file - methods are just exported and I never got around to fix it. Probably because I never managed to get an error with it. Now indeed with requireNamespace("pROC") it is clearly broken.

I created a branch s3_methods_namespace where this should now be fixed. Could you please confirm? This version can be installed for example with:

# install.packages("devtools")
library(devtools)
install_github("xrobin/pROC@s3_methods_namespace")

I will need more testing to make sure some other code (in pROC and the packages that depend on it) isn't broken before it can be released, but I'll do that as soon as possible.

berndbischl commented 9 years ago

Hi + Thx. I tested it with the branch version, and it seems to be OK.

Please close this when you update on CRAN so I know when to depend on a new version in our DESCRIPTION.

xrobin commented 9 years ago

Right now this is breaking the package RcmdrPlugin.ROC. I emailed the maintainer and hope he can update it soon. I will also email the maintainer of the other packages that depend on pROC in the next few days to inform them of the change so they can check their code beforehand.

xrobin commented 9 years ago

RcmdrPlugin.ROC is fixed but I just discovered an other issue, in the caret package this time. Pull request sent to topepo/caret#125.

topepo commented 9 years ago

I've merged your pull request. Thanks for doing that.

I'm working on a release of caret and want to use the dependency pROC (>= 1.8). When do you think that this will be on CRAN?

Thanks,

Max

xrobin commented 9 years ago

Thanks for merging the pull request.

As soon as the pull request is on CRAN I will try to push the 1.8.

Unfortunately it can't be the other way around, as CRAN would never accept an update breaking caret (or any other package for that matter). One option would be a to submit both updates at the same time.

Please note that the pull request I submitted will work with any version of pROC from 1.0, so there is no need to depend on 1.8.

topepo commented 9 years ago

Let's submit at the same time and send them a message beforehand. I've got a few outstanding things to finish before the next version and I think I'll be done in the next two weeks. How does that sound?

xrobin commented 9 years ago

Ok, let me know a few days before so we can land it. I may probably submit a bit earlier, if I tell the CRAN maintainers that you'll fix caret right after they'll probably accept the new version of pROC.

xrobin commented 9 years ago

Just discovered an additional error when testing caretEnsemble (zachmayer/caretEnsemble#135). I had mistakenly thought it was related with the issue mentioned earlier in caret.

topepo commented 9 years ago

I'm ready for a new CRAN version for caret. Are you geared up for a pROC submission? I can email CRAN and explain the situation in the meantime (unless you would rather communicate the issue).

Thanks,

Max

xrobin commented 9 years ago

Yeah I'm basically ready (I believe I can go ahead despite zachmayer/caretEnsemble#135, that shouldn't be a blocker). Pretty much any time this week would be good (TZ GMT+2).

topepo commented 9 years ago

Okay. I'll send CRAN a message saying that your pROC version 1.8 will break caret checks but I have a fixed version of caret that i will send in as soon as pROC is accepted.

Thanks for your help,

Max

xrobin commented 9 years ago

v1.8 submitted to CRAN with the note about caret, among others. Hopefully it will go through.

xrobin commented 9 years ago

I'm sorry the submission was rejected as there is a NOTE in R CMD check that I documented, but apparently the CRAN team missed the comment - at least I hope so.

topepo commented 9 years ago

No problem. version 1.8 is on CRAN and I'm getting my stuff together to submit.

Thanks,

Max

On Tue, May 5, 2015 at 2:10 AM, Xavier Robin notifications@github.com wrote:

I'm sorry the submission was rejected as there is a NOTE in R CMD check that I documented, but apparently the CRAN team missed the comment - at least I hope so.

— Reply to this email directly or view it on GitHub https://github.com/xrobin/pROC/issues/4#issuecomment-98965091.

xrobin commented 9 years ago

I insisted a bit and they published it.

So I guess that's it, thanks for your patience.

topepo commented 9 years ago

The new version of caret is on CRAN now.

Thanks,

Max

On Tue, May 5, 2015 at 9:38 AM, Xavier Robin notifications@github.com wrote:

I insisted a bit and they published it.

So I guess that's it, thanks for your patience.

— Reply to this email directly or view it on GitHub https://github.com/xrobin/pROC/issues/4#issuecomment-99079999.