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

Stability of layer data attributes #6194

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

Conversation

teunbrand
Copy link
Collaborator

This PR aims to fix #3175.

It avoids dropping attributes in some places and actively restores them in other places.
Attributes should now be stable from the moment aesthetics are computed up to the Geom$draw_*() methods.

@teunbrand
Copy link
Collaborator Author

I cannot figure out why coverage is failing and R CMD check passes.

@yutannihilation
Copy link
Member

I too cannot figure out, but I confirmed this error is reproducible on my local. As it looks too cryptic, maybe we can just ignore the failing tests for now? cf. https://covr.r-lib.org/#exclusions

══ Failed tests ════════════════════════════════════════════════════════════════
── Failure ('test-layer.R:177:3'): attributes on layer data are preserved ──────
attr(ld, "test") (`actual`) not equal to "preserve me" (`expected`).

`actual` is NULL
`expected` is a character vector ('preserve me')
── Failure ('test-scales.R:71:3'): identity scale preserves input values ───────
`d1` (`actual`) not equal to `d2` (`expected`).

     names(actual) | names(expected)     
 [1] "shape"       - "size"          [1] 
 [2] "size"        - "alpha"         [2] 
 [3] "alpha"       - "x"             [3] 
 [4] "x"           - "y"             [4] 
 [5] "y"           - "colour"        [5] 
 [6] "colour"      - "fill"          [6] 
 [7] "fill"        - "shape"         [7] 
 [8] "PANEL"       | "PANEL"         [8] 
 [9] "group"       | "group"         [9] 
[10] "stroke"      | "stroke"        [10]

[ FAIL 2 | WARN 1 | SKIP 258 | PASS 1593 ]

@teunbrand
Copy link
Collaborator Author

teunbrand commented Dec 2, 2024

I think I've made some debugging process. I think for some reason, ScalesList$transform_df() correctly tells the plot to skip on identity transformations in most contexts, but (for reasons beyond my current understanding) we do not skip this in the coverage test. The clue for this comes from #6076, where identity scales protest in check_transformation(), though it has no business checking identity transformations.

(df = df) at ggplot2/R/scales-.R:102:5
 11.                 └─ggplot2 (local) transform_df(..., self = self) at ggplot2/R/ggproto.R:188:17
 12.                   └─base::lapply(df[aesthetics], self$transform) at ggplot2/R/scale-.R:520:5
 13.                     └─ggplot2 (local) FUN(X[[i]], ...)
 14.                       └─ggplot2 (local) transform(..., self = self) at ggplot2/R/ggproto.R:[188](https://github.com/tidyverse/ggplot2/actions/runs/11951275132/job/33694697269?pr=6194#step:5:189):17
 15.                         └─ggplot2:::check_transformation(...) at ggplot2/R/scale-.R:640:3

So this laid bare that I hadn't included scale transformations as preserving attributes, and now this is included :)

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 this pull request may close these issues.

Access to layer information during build stages
2 participants