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

Warn when a supplied mapping is going to be overwritten by geom_hline / vline / abline #2950

Merged
merged 15 commits into from Jan 23, 2019

Conversation

@RichardJActon
Copy link
Contributor

@RichardJActon RichardJActon commented Oct 19, 2018

This pull request addressed issue #2945

geom_hline / vline / abline will now throw a warning if the user supplies both a x|y|intercept/slope value and a mapping such as this:

geom_hline(yintercept = 3, aes(colour = colour))

As the above will cause the user supplied mapping to be ignored/overwritten

Users might resaonably expect the above to work as other geoms do not have this behaviour. The warning messages suggest that the x|y|intercept/slope value(s) be moved inside the aes call thus:

geom_hline(aes(yintercept = 3,colour = colour))
R/geom-hline.r Outdated Show resolved Hide resolved
@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Oct 20, 2018

Is it OK to recommend to place values directly in aes()? In this case, it's fine, but I'm a bit afraid this may be a bit confusing because using direct values in aes() is not always good.

Maybe preparing data is the right thing? Or considering the hacky nature of the "annotation mode", is this allowed specially?

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Oct 20, 2018

Maybe preparing data is the right thing?

Actually, geom_hline() will overwrite the dataset if one is provided, so this won't work. I've been wondering whether the code block here:

ggplot2/R/geom-hline.r

Lines 13 to 17 in 3550772

if (!missing(yintercept)) {
data <- data.frame(yintercept = yintercept)
mapping <- aes(yintercept = yintercept)
show.legend <- FALSE
}

should only be executed when data is NULL.

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Oct 20, 2018

I meant not providing *intercept and maping all the variables.

data2 <- data
data2$y <- 3

geom_hline(data = data2,
           aes(yintercept = y, colour = colour))

But, please ignore my comment above. I think I don't get the setantics of variable mappings yet. Sorry for the noise.

R/geom-hline.r Outdated Show resolved Hide resolved
R/geom-hline.r Outdated Show resolved Hide resolved
tests/testthat/test-geom-hline-vline-abline.R Outdated Show resolved Hide resolved
@RichardJActon
Copy link
Contributor Author

@RichardJActon RichardJActon commented Oct 30, 2018

Are any further changes needed here?

  • I have placed the warning block in the existing check for a missing intercept value and removed the redundant test for the intercept.
  • I removed the plot from the test and just checked the geoms directly
  • I altered the error message so it does suggest the solution of placing a value directly in a mapping

(I'm not sure what's up with travis - the error says something about exceeding the API rate limit?)

Copy link
Member

@clauswilke clauswilke left a comment

I made a few specific comments. We would also need an entry in NEWS.md.

R/geom-abline.r Outdated Show resolved Hide resolved
R/geom-abline.r Show resolved Hide resolved
tests/testthat/test-geom-hline-vline-abline.R Outdated Show resolved Hide resolved
@clauswilke
Copy link
Member

@clauswilke clauswilke commented Jan 20, 2019

This cannot currently be merged because the code in geom_abline(), geom_hline(), and geom_vline() has changed.

@RichardJActon Would you mind updating this? Could you please also add a bullet to NEWS.md? It should briefly describe the change and end with (@yourname, #issuenumber).

@RichardJActon
Copy link
Contributor Author

@RichardJActon RichardJActon commented Jan 21, 2019

Hi @clauswilke,
Was there a concensus for implementing your suggestion to only overwrite the data if none is provided?

At present with just adding the warning message this won't break anyones code but if someone has supplied data that is currently being overwritten silently this change might cause them issues.

I expect I can update this later today but do we want it with or without the change to the data overwrite behaviour?

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Jan 21, 2019

We just opened a separate issue about the overwriting of data: #3090 I suggest you finish this PR as it currently stands.

tests/testthat/test-geom-hline-vline-abline.R Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
@clauswilke clauswilke merged commit 1f259f4 into tidyverse:master Jan 23, 2019
4 checks passed
@clauswilke
Copy link
Member

@clauswilke clauswilke commented Jan 23, 2019

Thanks!

@lock
Copy link

@lock lock bot commented Jul 22, 2019

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/

@lock lock bot locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants