eliocamp / ggnewscale

Multiple Fill, Color and Other Scales in `ggplot2`
https://eliocamp.github.io/ggnewscale/
GNU General Public License v3.0
406 stars 18 forks source link

`new_scale` modifies the ggplot object #13

Closed raymondben closed 5 years ago

raymondben commented 5 years ago

This might be intended behaviour, in which case just close the issue - but it caught me out! I didn't find it mentioned in the docs, but maybe I missed it? Adding new_scale to a ggplot object modifies that object. In some cases this caused errors for me in later code, because the ggplot object has changed unexpectedly.

Should p + new_scale() change the p object, if it isn't being assigned back to p?

library(ggplot2)
library(ggnewscale)
topography <- expand.grid(x = 1:nrow(volcano), y = 1:ncol(volcano))
topography$z <- c(volcano)

## create two equivalent ggplot objects
p0 <- ggplot(mapping = aes(x, y)) + geom_contour(data = topography, aes(z = z, color = stat(level))) + scale_color_viridis_c(option = "D")
p <- ggplot(mapping = aes(x, y)) + geom_contour(data = topography, aes(z = z, color = stat(level))) + scale_color_viridis_c(option = "D")

all.equal(p, p0)
[1] TRUE
## ok, as expected they are the same

## now add new scale to p, but WITHOUT explicitly assigning back to `p`
p + new_scale_colour()

all.equal(p, p0)
 [1] "Component “layers”: Component 1: Component 3: Names: 1 string mismatch"                                         
 [2] "Component “layers”: Component 1: Component 3: Length mismatch: comparison on first 2 components"                
 [3] "Component “layers”: Component 1: Component 3: Component 1: Names: 1 string mismatch"                            
 [4] "Component “layers”: Component 1: Component 3: Component 2: target, current do not match when deparsed"          
 [5] "Component “layers”: Component 1: Component 6: Names: 1 string mismatch"                                         
 [6] "Component “layers”: Component 1: Component 9: Names: 4 string mismatches"                                       
 [7] "Component “layers”: Component 1: Component 9: Length mismatch: comparison on first 4 components"                
 [8] "Component “layers”: Component 1: Component 9: Component 1: Modes: list, function"                               
 [9] "Component “layers”: Component 1: Component 9: Component 1: names for target but not for current"                
[10] "Component “layers”: Component 1: Component 9: Component 1: Attributes: < Modes: list, NULL >"                   
[11] "Component “layers”: Component 1: Component 9: Component 1: Attributes: < Lengths: 1, 0 >"                       
[12] "Component “layers”: Component 1: Component 9: Component 1: Attributes: < names for target but not for current >"
[13] "Component “layers”: Component 1: Component 9: Component 1: Attributes: < current is not list-like >"            
[14] "Component “layers”: Component 1: Component 9: Component 1: current is not list-like"                            
[15] "Component “layers”: Component 1: Component 9: Component 2: Modes of target, current: function, list"            
[16] "Component “layers”: Component 1: Component 9: Component 2: target, current do not match when deparsed"          
[17] "Component “layers”: Component 1: Component 9: Component 3: Lengths (0, 3) differ (string compare on first 0)"   
[18] "Component “layers”: Component 1: Component 9: Component 4: target is NULL, current is function"    
eliocamp commented 5 years ago

This is not intended. Thanks for reporting. It seems I'm being foiled by unexpected modify by reference.

eliocamp commented 5 years ago

All should be good now.

library(ggplot2)
library(ggnewscale)
topography <- expand.grid(x = 1:nrow(volcano), y = 1:ncol(volcano))
topography$z <- c(volcano)

## create two equivalent ggplot objects
p0 <- ggplot(mapping = aes(x, y)) + geom_contour(data = topography, aes(z = z, color = stat(level))) + scale_color_viridis_c(option = "D")
p <- ggplot(mapping = aes(x, y)) + geom_contour(data = topography, aes(z = z, color = stat(level))) + scale_color_viridis_c(option = "D")

p + new_scale_colour()
#> Registered S3 method overwritten by 'ggedit':
#>   method from   
#>   +.gg   ggplot2


all.equal(p, p0)
#> [1] TRUE

Created on 2019-07-15 by the reprex package (v0.3.0)

raymondben commented 5 years ago

Super! Thanks

eliocamp commented 5 years ago

Thanks to you for reporting the bug :D

eliocamp commented 5 years ago

The fix for this bug introduced more serious bugs (see #14), so I'm reverting the fix and looking for other options. Sorry, @raymondben!

eliocamp commented 5 years ago

Now it's got to be fixed.

raymondben commented 5 years ago

Heheh. If a job's worth doing, it's worth doing twice, yeah? Thanks again.