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

Release v3.4.0 #4854

Closed
thomasp85 opened this issue May 20, 2022 · 36 comments
Closed

Release v3.4.0 #4854

thomasp85 opened this issue May 20, 2022 · 36 comments
Milestone

Comments

@thomasp85
Copy link
Member

Hi team @clauswilke @yutannihilation @paleolimbot @karawoo @hadley

I'm beginning to get the next release in shape. Once again this release will not be feature heavy. It will however focus on getting a lot of the internals in line as well as improving the API (with the potential of it causing some breaking changes)

I've updated to the stale 3.4.0 milestone, removing issues I don't think fit in. If any of you have something you want to work on and get into this release, please add it to the milestone so we can keep track of it

There is no release date yet but I would like to get this done at least before the end of summer

@thomasp85 thomasp85 added this to the ggplot2 3.4.0 milestone May 20, 2022
@thomasp85
Copy link
Member Author

I think it could be nice to finally pull the trigger on switching to vctrs. @hadley made some experimentation for this some time ago in #4342 - do you have any recollection?

@clauswilke
Copy link
Member

I think I can cover the two issues that are assigned to me in the 3.4.0 milestone. I should be able to do that within the next few days.

@hadley
Copy link
Member

hadley commented May 20, 2022

@thomasp85 I think the vctrs PR needs quite a lot more work to be able to tell what the downstream implications are — it's possible that it has limited impacts to the user, and could be bundled in this release, but it's also possible that it will affect a decent amount of code, and we'll need to either put it in a big release or some how make it optional.

@yutannihilation
Copy link
Member

As the previous comment on the pull request says it would be "for the next big ggplot2 release," I too thought so.

@yutannihilation
Copy link
Member

Can we add #3818 to the milestones? While I'm honestly not happy with the consequence of exposing both add_ggplot() and ggplot_add(), which looks confusing, I think we should merge the pull request if we decided to go in the direction. I don't see any blockers on including this in the next release.

@thomasp85
Copy link
Member Author

@yutannihilation i think I have kicked the PR down the road because I ultimately don't think I want to expose it but have also failed with coming up with a concise reason for it. But I agree we should at least make a decision about it prior to this release

@yutannihilation
Copy link
Member

yutannihilation commented May 21, 2022

Ah, okay. Since hadley agreed on the direction (#3818 (comment), and #4743 (comment)) and no one has raised objections so far, I thought it was the decision. Let's discuss there if needed.

@davidhodge931
Copy link

Any eta about when you guys are planning on releasing v3.4.0 of ggplot2? Just thinking about how supporting the linewidth thing is going to happen in my extension package

@thomasp85
Copy link
Member Author

OK, we seem to be very close to starting release process. @clauswilke and @yutannihilation do you have any issues or PRs not tagged with the milestone you feel needs to get into this release?

@thomasp85
Copy link
Member Author

@davidhodge931 I believe august is a fair aim for this release. I'll work on an article for extension package developers helping them transition

@clauswilke
Copy link
Member

@thomasp85 I don't.

@yutannihilation
Copy link
Member

Thanks, I added two.

@yutannihilation
Copy link
Member

I found #3816 could be the one that the next release is the good timing to release, but it might be a bit too late...

@yutannihilation
Copy link
Member

I added #4953 to the milestone because this is probably what we should decide before releasing linewidth.

@thomasp85
Copy link
Member Author

Thanks @yutannihilation - let's freeze the milestone at this point then... I've been distracted by CRANs move to HTML5 but I'm back working on this release now

@thomasp85
Copy link
Member Author

Hi Everyone. The milestone has cleared and I'll make a release branch and begin the release procedure. Thanks everyone for their hard work 🎉❤️

@yutannihilation
Copy link
Member

Glad that we made it! Please let me know if there's anything I can help in investigating revdeps.

@thomasp85
Copy link
Member Author

Hi Team

Some feedback from the spatial community about the size to linewidth change which will unfortunately break the visuals of many of their plots. Should we take this opportunity to change the default linewidth in geom_sf to something thinner? It seems the only reason it is as it is is so that points are not too small? The defaults for lines was designed for line charts and when drawing country borders it is quite heavy so I feel a better default in sf is possible now that it is decoupled from points

@yutannihilation
Copy link
Member

I agree a thinner default should be better for polygon edges. Let me confirm, are you suggesting to change only the default linewidth of polygon features, not that of line features?

@clauswilke
Copy link
Member

I always reduce the thickness of polygon edges when using geom_sf(), so yes, a thinner default seems reasonable. I never plot line features so I have no opinion on that.

@thomasp85
Copy link
Member Author

We can't do that unfortunately - it would have to be changing both for lines and polygons

@yutannihilation
Copy link
Member

yutannihilation commented Aug 26, 2022

We can't do that

Really? But, isn't the default determined base on the type of the feature...? I thought it would be not very difficult to tweak this defaults here.

linewidth <- x$linewidth %||% defaults$linewidth[type_ind]

I think it's a fair bet that most of the usage of geom_sf() is to draw polygons, but I hope we can let users to draw nice lines if possible. I sometimes plot roads and rivers.

@thomasp85
Copy link
Member Author

Oh yeah, maybe I spoke too soon. Will give it a go

@thomasp85
Copy link
Member Author

But thinking about it, that would mean that strokes are a different width only when using defaults which is perhaps not the most intuitive thing. Consider a user looking at the default output and thinking "I wish the borders where thicker" goes on to set the line width and now suddenly both borders and lines change

@yutannihilation
Copy link
Member

I'm not sure, but I think polygons and lines are on different layers in most cases, so that surprise should be rare.

@yutannihilation
Copy link
Member

To me, the main reason why it's justifiable to have a different default linewidth than other polygon-based geom is that it's easier to distinguish line features and polygon edges. So, I think lines should have different default.

Anyway, I'm not very familiar with GIS things. So, please ignore me if you are confident.

@FelixErnst
Copy link

FelixErnst commented Oct 3, 2022

@thomasp85 Through the dependency ggplot2 --> ggnewscale --> miaViz I was informed today that ggnewscale would be archived the 17th of October due to the note regarding incompatibility to HTML5 standards. The issue is a note generated on r-devel, which look like this also found in the build report of ggplot2 on CRAN:

image

  • Does ggplot2 fix those NOTEs on CRAN?
  • Any chance ggplot2 3.4 will have hit CRAN by then?

Thanks for a comment in advance.

@antagomir @TuomasBorman

@thomasp85
Copy link
Member Author

I'm in contact with CRAN and they have given it to the end of October for release so this is surprising. I'll reach out to them

@FelixErnst
Copy link

FelixErnst commented Oct 3, 2022

Thank you. I discussed this and made them aware of this via mail. Uwe and Kurt are in the loop on this.

@thomasp85
Copy link
Member Author

Hi Team

All revdep issues have been taken care of. It was quite a lot of work, even by ggplot2 standards, but 3.4.0 is now on track for a release by the end of October to allow for packages to respond.

As part of the revdepcheck process the RC branch has been updated quite a lot and I'll briefly go through some of the changes below:

  • The mapped_discrete class which has been ported to use vctrs is back as a non-voters class. We found that vctrs was too strict for what people were doing in their extensions.
  • All new deprecations are soft to give developers time to adjust without too much fuss
  • There are now additional gaurd-rails for the size->linewidth switch so even fewer packages are affected
  • We now use a less strict version of vec_rbind(), again because we see a bit too many failures with the strictness imposed by vctrs. The issues are all related to type inconsistency in faceting variables across layers. These situations all work again but will throw a deprecation warning
  • The rest is standard fixes to issues that cropped up with the extended testing that revdeps provide

@thomasp85
Copy link
Member Author

Also, @yutannihilation and @clauswilke, in relation to our talk about changing the default geom_sf() linewidth. I'm considering setting this to 0.2. I feel 0.1 is slightly too thin, but I'm open to being convinced otherwise. Thoughts?

@clauswilke
Copy link
Member

Yes, agreed. In particular with the default theme, 0.1 is too thin.

library(ggplot2)

nc <- sf::st_read(system.file("shape/nc.shp", package = "sf"), quiet = TRUE)

# current default (size = 0.5?)
ggplot(nc) +
  geom_sf(color='black')

# 0.2
ggplot(nc) +
  geom_sf(color='black', size = 0.2)

# 0.1
ggplot(nc) +
  geom_sf(color='black', size = 0.1)

Created on 2022-10-11 by the reprex package (v2.0.1)

@yutannihilation
Copy link
Member

My opinion stays the same as above (lines and polygon outlines should have different widths), but I'm fine with your decision.

@thomasp85
Copy link
Member Author

Lines and polygons will have different defaults. The above is to find the right new default for polygons. My suggestion is 0.2

@yutannihilation
Copy link
Member

Oh, then, it sounds perfect to me!

@teunbrand
Copy link
Collaborator

It feels safe to close this issue

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

No branches or pull requests

7 participants