topepo / caret

caret (Classification And Regression Training) R package that contains misc functions for training and plotting classification and regression models
http://topepo.github.io/caret/index.html
1.62k stars 633 forks source link

R CMD check and dev ggplot2 #317

Closed hadley closed 9 years ago

hadley commented 9 years ago

I see:

checking S3 generic/method consistency ... WARNING
ggplot:
  function(data, mapping, ..., environment)
ggplot.rfe:
  function(data, metric, output, ...)

ggplot:
  function(data, mapping, ..., environment)
ggplot.train:
  function(data, metric, plotType, output, nameInStrip, highlight, ...)

ggplot:
  function(data, mapping, ..., environment)
ggplot.train:
  function(data, metric, plotType, output, nameInStrip, highlight, ...)

ggplot:
  function(data, mapping, ..., environment)
ggplot.rfe:
  function(data, metric, output, ...)

See section ‘Generic functions and methods’ in the ‘Writing R
Extensions’ manual.

Can you please look into ASAP? I'm submitting to CRAN on Friday.

topepo commented 9 years ago

It looks like the generic is

ggplot <- function(data = NULL, mapping = aes(), ...,
                   environment = parent.frame()) {
  UseMethod("ggplot")
}

Would it solve the issue by redefining

ggplot.rfe <- function(data,
                       mapping = NULL, 
                       metric = data$metric[1], 
                       output = "layered", 
                       ..., 
                       environment = NULL)

instead of

ggplot.rfe <- function (data = NULL, metric = data$metric[1], output = "layered", ...) 

(and similar changes for the others)

I think order matters here, which is why I ordered the arguments to ggplot.rfe in that manner.

I'm not sure when I can submit a new version of caret. I've done two in the last month (the last to fix a critical bug). When I submitted the last one, I expected to see a mushroom cloud over 51.7611° N, 1.2534° W.

hadley commented 9 years ago

I don't think the order matters - the problem is that you're missing the environment argument.

When you resubmit, just say its for compatibility with the next version of ggplot2, and you should be ok.

topepo commented 9 years ago

Okay. I think I need both mapping and environment added.

I will start the testing/release process for the new caret version. It will probably get submitted Sunday-ish. It should probably come after yours is on CRAN since I'll add a version requirement in the description file.

On Wed, Nov 11, 2015 at 11:19 AM, Hadley Wickham notifications@github.com wrote:

I don't think the order matters - the problem is that you're missing the environment argument.

When you resubmit, just say its for compatibility with the next version of ggplot2, and you should be ok.

— Reply to this email directly or view it on GitHub https://github.com/topepo/caret/issues/317#issuecomment-155832782.

hadley commented 9 years ago

Per my email, I'm now not planning to submit until Dec 11. If possible could you please submit a version of caret that works with both released and dev versions of ggplot2? I think the changes will be ok with the current dev version.

topepo commented 9 years ago

Okay. I saw

I'm submitting to CRAN on Friday.

but I didn't see a separate email.

I'll test with both versions before I submit (before 12/11). I'll keep you posted on this thread as well as the CRAN submission.

hadley commented 9 years ago

Maybe you're not the official maintainer?

topepo commented 9 years ago
> packageDescription("caret")$Maintainer
[1] "Max Kuhn <Max.Kuhn@pfizer.com>"

Most likely it got scrubbed by spam filters although I've gotten message form you before. What email did it come from? I'll white-list it.

hadley commented 9 years ago

Weird - it's from h.wickham@gmail.com. It's possible that because I'm listing everyone in bcc, it gets cut off?

topepo commented 9 years ago

Ah. It is under spam but your other email from the 5th is not. I'll add it to the "do not delete" list.

A few years ago, it classified emails from one of my subordinates as spam. If only we knew something about predictive models =]

topepo commented 9 years ago

I tested with both versions so it should be fine. I'll submit it this week.

Thanks,

Max