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

NAMESPACE / S3 #4

Closed
berndbischl opened this issue Dec 17, 2014 · 17 comments · Fixed by topepo/caret#125
Closed

NAMESPACE / S3 #4

berndbischl opened this issue Dec 17, 2014 · 17 comments · Fixed by topepo/caret#125
Assignees
Milestone

Comments

@berndbischl
Copy link

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
Copy link
Owner

xrobin commented Feb 7, 2015

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
Copy link
Author

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
Copy link
Owner

xrobin commented Feb 9, 2015

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
Copy link
Owner

xrobin commented Feb 27, 2015

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
Copy link

topepo commented Apr 19, 2015

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
Copy link
Owner

xrobin commented Apr 19, 2015

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
Copy link

topepo commented Apr 19, 2015

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
Copy link
Owner

xrobin commented Apr 20, 2015

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
Copy link
Owner

xrobin commented Apr 20, 2015

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
Copy link

topepo commented Apr 30, 2015

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
Copy link
Owner

xrobin commented May 3, 2015

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
Copy link

topepo commented May 3, 2015

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
Copy link
Owner

xrobin commented May 4, 2015

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

@xrobin
Copy link
Owner

xrobin commented May 5, 2015

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
Copy link

topepo commented May 5, 2015

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
#4 (comment).

@xrobin
Copy link
Owner

xrobin commented May 5, 2015

I insisted a bit and they published it.

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

@xrobin xrobin closed this as completed May 5, 2015
@topepo
Copy link

topepo commented May 6, 2015

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
#4 (comment).

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 a pull request may close this issue.

3 participants