Skip to content

Guides for coord_sf() #5293

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

Merged
merged 10 commits into from
May 24, 2023
Merged

Guides for coord_sf() #5293

merged 10 commits into from
May 24, 2023

Conversation

teunbrand
Copy link
Collaborator

This PR aims to fix a problem akin to #3959, but for coord_sf().

Briefly, coord_sf() was doing its own guide thing, ignoring any user-provided guides.
Now, it renders the guides through the guides system.

Less briefly:
To make this work, I had to cast the graticule positions into a viewscale that the guides can use for training.
I was a bit surprised that axis placement is tightly controlled in coord_sf() itself, in that scale_x_continuous(position = "top") is ignored and just adding guides(x.sec = "axis") doesn't just set a top axis.
Maybe it would be good to allow some flexibility here, but I didn't change that behaviour in this PR.
I'm new-ish to {sf}, so if anybody knows of hairy edge cases I mightn't have thought of, please let me know.

To illustrate:

devtools::load_all("~/packages/ggplot2/")
#> ℹ Loading ggplot2

nc <- sf::st_read(system.file("shape/nc.shp", package = "sf"), quiet = TRUE)
ggplot(nc) +
  geom_sf() +
  coord_sf(crs = 3347, label_graticule = c("NSWE")) + # all labels
  guides(
    x = guide_none("guide_none() passes on their title"),
    y = guide_axis("angles work", angle = 45),
    x.sec = guide_axis("dodging works", n.dodge = 2),
    y.sec = guide_axis(angle = -45, n.dodge = 2)
  )

Created on 2023-05-01 with reprex v2.0.2

@thomasp85 thomasp85 requested a review from clauswilke May 22, 2023 08:50
@thomasp85
Copy link
Member

@clauswilke you have spend the most time with sf - do you have the bandwidth to look at this?

@clauswilke
Copy link
Member

@thomasp85 Yes, this touches mostly code that I wrote, so I guess I'll have to look at it carefully. I don't remember much of it, but I'll take a look.

@teunbrand Two quick items from my end:

  1. Are we using Map() in ggplot? I thought we're generally using apply functions.
  2. It looks to me like a bunch of comments have disappeared in porting the code. I would encourage you to comment a bit more generously. This is confusing code, and without comments nobody will understand what it does five years into the future.

@clauswilke
Copy link
Member

To be clear: My code already didn't have that many comments. The new code has even fewer. I think the goal should be more comments than my original code had.

@teunbrand
Copy link
Collaborator Author

teunbrand commented May 22, 2023

Are we using Map() in ggplot? I thought we're generally using apply functions.

Under the hood, Map() uses the apply-family of functions (mapply) but doesn't simplify output. Should there be a reason not to use it?

I think the goal should be more comments than my original code had.

Yeah this is a fair point and had some problem deciphering which parts were doing what as well.

@clauswilke
Copy link
Member

Regarding Map(), I'll leave it to @thomasp85 to make this decision. As a general principle, since we're writing ggplot2 using somewhat obscure (for people coming from the tidyverse) base-R functionality, it makes sense to use a limited set of functions to keep the code easier to read. At least I always thought that we're doing this. I could be wrong.

@teunbrand
Copy link
Collaborator Author

teunbrand commented May 23, 2023

I've sprinkled around some comments and improved some variable names. I'm at such familiarity with GIS that I have to google every time what a meridian and parallel is, so bare with me if I get them confused at times.

Copy link
Member

@clauswilke clauswilke left a comment

Choose a reason for hiding this comment

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

I've reviewed it and had two more minor comments. Overall this seems fine to me, though I will say I don't fully understand the guide system. In the long run, this will likely be more maintainable, though, so moving forward in this direction is good.

@teunbrand
Copy link
Collaborator Author

Thanks for the review Claus! Yeah I suppose the guide system will take some getting used to, but I'd be happy to try to answer any questions you might have.

@teunbrand teunbrand merged commit 49d13ea into tidyverse:main May 24, 2023
@teunbrand teunbrand deleted the sf_guides branch May 24, 2023 20:40
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.

3 participants