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

R CMD check and dev ggplot2 #317

Closed
hadley opened this issue Nov 10, 2015 · 10 comments
Closed

R CMD check and dev ggplot2 #317

hadley opened this issue Nov 10, 2015 · 10 comments

Comments

@hadley
Copy link

@hadley hadley commented Nov 10, 2015

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

@topepo topepo commented Nov 11, 2015

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

@hadley hadley commented Nov 11, 2015

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

@topepo topepo commented Nov 11, 2015

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

@hadley
Copy link
Author

@hadley hadley commented Nov 11, 2015

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

@topepo topepo commented Nov 11, 2015

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

@hadley hadley commented Nov 11, 2015

Maybe you're not the official maintainer?

@topepo
Copy link
Owner

@topepo topepo commented Nov 11, 2015

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

@hadley hadley commented Nov 11, 2015

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

@topepo
Copy link
Owner

@topepo topepo commented Nov 11, 2015

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 added a commit that referenced this issue Nov 12, 2015
Checked on ggplot2_1.0.1.9003 (github) and ggplot2_1.0.1 (CRAN)
@topepo
Copy link
Owner

@topepo topepo commented Nov 16, 2015

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

Thanks,

Max

@topepo topepo closed this Nov 16, 2015
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
2 participants
You can’t perform that action at this time.