Skip to content

geom_contour() documentation states false precedence of bin and binwidth parameters #4651

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

Closed
eliocamp opened this issue Oct 28, 2021 · 2 comments · Fixed by #4720
Closed

Comments

@eliocamp
Copy link
Contributor

Right now the documentation for geom_contour() and friends states that the bin argument is overridden by the binwidth parameter. However, this seems not to be the case.

library(ggplot2)


v <- ggplot(faithfuld, aes(waiting, eruptions, z = density)) 

# Defining bins
v + geom_contour(bins = 5)

# Defining binwidth
v + geom_contour(binwidth = 0.001)

# Defining both. Per documentation, binwidth should 
# take precedence and this should be the same as the second plot
v + geom_contour(binwidth = 0.001, bins = 5)

Created on 2021-10-28 by the reprex package (v2.0.0)

The issue lays on the contour_breaks() function, which computes the binwidth from the bins if bins is not NULL:

ggplot2/R/stat-contour.r

Lines 158 to 170 in 759c63c

if (!is.null(bins)) {
# round lower limit down and upper limit up to make sure
# we generate bins that span the data range nicely
accuracy <- signif(diff(z_range), 1)/10
z_range[1] <- floor(z_range[1]/accuracy)*accuracy
z_range[2] <- ceiling(z_range[2]/accuracy)*accuracy
if (bins == 1) {
return(z_range)
}
binwidth <- diff(z_range) / (bins - 1)
breaks <- fullseq(z_range, binwidth)

I believe that previous versions of geom_contour() did respect the documentation. Should the documentation be changed to reflect the new precedence or should the code be changed to reflect the documentation?

@eliocamp eliocamp changed the title geom_contour() documentation sta geom_contour() documentation states false precedence of bin and binwidth parameters Oct 28, 2021
@thomasp85
Copy link
Member

@clauswilke is this a leftover bug from the contour rewrite?

@clauswilke
Copy link
Member

Possibly, though it's also possible it was never right. There's no technical reason I can see why the contour rewrite would have touched this part.

In any case, I have no strong opinion about the order of precedence, but I agree we should fix this. I'd vote for fixing the documentation. Let's not touch the code unless there's a good reason to do so.

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