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.

Sign up for GitHub

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

Compatability with ggplot2 3.6.0 #166

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Compatability with ggplot2 3.6.0 #166

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d0519a0
remove test
teunbrand Jan 30, 2025
5e30828
target adding labs
teunbrand Jan 30, 2025
ef4b3d5
Prioritise `+.gg_tsensemble` method over `+.gg` to resolve Ops conflict
mitchelloharawild Jan 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 1 addition & 8 deletions tests/testthat/test-graphics.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ test_that("gg_tsdisplay() plots", {
list(x = "index", y = "value")
)

p <- p + ggplot2::labs(x = "x", y = "y", title = "title")
p[[1]] <- p[[1]] + ggplot2::labs(x = "x", y = "y", title = "title")

p_built <- ggplot2::ggplot_build(p[[1]])

Expand Down Expand Up @@ -267,11 +267,4 @@ test_that("gg_arma() plots", {
ggplot2::layer_data(p, 4)$PANEL,
factor(c(rep_along(ar_roots, 1), rep_along(ma_roots, 2)))
)

p_built <- ggplot2::ggplot_build(p)

expect_equivalent(
p_built$plot$labels[c("x", "y")],
list(x = "Re(1/root)", y = "Im(1/root)")
)
Comment on lines -270 to -276
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expected label is derived from the global mapping of the plot, while ggplot2 now prioritises layers for default mappings (so the path geoms is the one to provide x/y).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, in this case the code is better updated to return + ggplot2::labs(x = "Re(1/root)", y = "Im(1/root)").

Am I correct in understanding that ggplot(aes(x = mpg, y = hp)) will now return default axis labels of x = "x" and y = "y" rather than x = "mpg" and y = "hp"? Since geom_*() in this case is inheriting from the default ggplot aes(), it feels like a regression that the path geom wouldn't produce the same x/y labels.

Copy link
Contributor Author

@teunbrand teunbrand Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will now return default axis labels

It will be mpg/hp in that case. Essentially it goes over the layers one-by-one and picks up the labels, so the order in which aesthetics are evaluated matters. It ignores the global aesthetics because they only matter when they are inherited by a layer. The relevant issue is tidyverse/ggplot2#5894 if you want to read more.

Copy link
Member

@mitchelloharawild mitchelloharawild Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the reference! I'm not against the change, and I think the relevant issue is well motivated.

It seems there could be an issue with using annotate() and the new automatic layer-based labels (or I misunderstand their intended interaction -- I haven't read the relevant issue closely yet).

Here's an adapted version of gg_arma() that removes the model and the package NSE handling junk. The default aes(x = Re(1/root), y = Im(1/root), colour = UnitCircle) results in labels labs(x = "x", y = "y", colour = "UnitCircle") for these layers:

... +
  ggplot2::annotate(
    "path", x = cos(seq(0, 2 * pi, length.out = 100)),
    y = sin(seq(0, 2 * pi, length.out = 100))
  ) +
  ggplot2::geom_vline(xintercept = 0) +
  ggplot2::geom_hline(yintercept = 0) +
  geom_point()

Removing annotate() produces the expected labels labs(x = "Re(1/root)", y = "Im(1/root)", colour = "UnitCircle").

Full (not so minimal) MRE ({reprex} isn't working for me at the moment, so apologies for the botched output):

plot_data <- structure(list(
  type = structure(c(1L, 1L, 1L, 1L, 1L, 1L, 1L, 
                     1L, 1L, 1L, 1L, 1L, 1L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 
                     2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L), levels = c("AR roots", "MA roots"), class = "factor"), 
  root = c(0.743346174645867+0.743346174645867i, 
           -0.743346174645867+0.743346174645867i, -0.743346174645867-0.743346174645867i, 
           1.01542975837224-0.272083583726372i, 0.272083583726371+1.01542975837224i, 
           -1.01542975837224+0.272083583726371i, -0.272083583726371-1.01542975837224i, 
           1.01542975837224+0.272083583726372i, -0.272083583726372+1.01542975837224i, 
           -1.01542975837224-0.272083583726372i, 0.272083583726372-1.01542975837224i, 
           0.743346174645867-0.743346174645868i, 3.89255799705759+6.88853197873318e-16i, 
           0.7382028071715+0.7382028071715i, -0.877092143351749+0.506389385068237i, 
           -0.506389385068237-0.877092143351749i, 0.877092143351748-0.506389385068237i, 
           0.506389385068238+0.877092143351749i, -1.00840378774125+0.270200980569754i, 
           -0.27020098056976-1.00840378774126i, 1.01277877013646+2.37053503325252e-14i, 
           -6.48524393002789e-15+1.01277877013648i, -1.01277877013648+1.03769254626384e-15i, 
           0.270200980569773-1.00840378774126i, 1.00840378774127-0.270200980569749i, 
           0.270200980569755+1.00840378774126i, -1.00840378774126-0.270200980569754i, 
           3.1285544130455e-15-1.01277877013649i, 0.877092143351752+0.506389385068235i, 
           -0.50638938506824+0.877092143351747i, -0.877092143351755-0.506389385068242i, 
           0.506389385068258-0.877092143351732i, 1.00840378774125+0.270200980569741i, 
           -0.7382028071715+0.738202807171498i, -0.738202807171499-0.738202807171506i, 
           0.738202807171505-0.738202807171487i, -0.270200980569757+1.00840378774125i, 
           1.71020671745964+2.77960735720925e-15i), 
  UnitCircle = structure(c(2L, 
                           2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 
                           2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 
                           2L, 2L, 2L, 2L, 2L), levels = c("Outside", "Within"), class = "factor")),
  row.names = c(NA, -38L), class = c("tbl_df", "tbl", "data.frame"))


plot_data

library(ggplot2)
ggplot(plot_data, aes(x = Re(1/root), y = Im(1/root),
                      colour = UnitCircle)) +
  ggplot2::annotate(
    "path", x = cos(seq(0, 2 * pi, length.out = 100)),
    y = sin(seq(0, 2 * pi, length.out = 100))
  ) +
  ggplot2::geom_vline(xintercept = 0) +
  ggplot2::geom_hline(yintercept = 0) +
  geom_point() +
  ggplot2::coord_fixed(ratio = 1) +
  facet_grid(cols = vars(type))

image

Copy link
Member

@mitchelloharawild mitchelloharawild Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking into annotate() a bit more, since it is essentially setting up a geom_path() I can understand why it is producing x/y labels. However the resulting labels were a bit surprising to me before I looked into the code since annotate() doesn't have a traditional data+mapping syntax like other layer functions. It is probably difficult to implement, but perhaps annotate() layers should be ignored for layer-based labels.

Perhaps it is best if I just add the desired labels into the plot explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the suggestion and I've added it to tidyverse/ggplot2#6290

})