Release v3.4.0 - Githubissues

thomasp85 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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

yutannihilation commented 2 years ago

Can we add https://github.com/tidyverse/ggplot2/pull/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 commented 2 years ago

@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 commented 2 years ago

Ah, okay. Since hadley agreed on the direction (https://github.com/tidyverse/ggplot2/pull/3818#issuecomment-1076435320, and https://github.com/tidyverse/ggplot2/issues/4743#issuecomment-1066978477) and no one has raised objections so far, I thought it was the decision. Let's discuss there if needed.

davidhodge931 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@thomasp85 I don't.

yutannihilation commented 2 years ago

Thanks, I added two.

I'd like to include #4917 if possible, not because it's related to #4866, but because it's a breaking change. I'm not sure if we can agree on the change, but if so, it would be a bit too long to wait for another non-patch release.
4921 also looks nice to include in the release. But I think it's safe to have this in a patch release, so this is just a nice to have.

yutannihilation commented 2 years ago

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 commented 2 years ago

I added https://github.com/tidyverse/ggplot2/issues/4953 to the milestone because this is probably what we should decide before releasing linewidth.

thomasp85 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

thomasp85 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

yutannihilation commented 2 years ago

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.

https://github.com/tidyverse/ggplot2/blob/a58b48c961cb391b8646bf072b6620a0c9f3d999/R/geom-sf.R#L204

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 commented 2 years ago

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

thomasp85 commented 2 years ago

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 commented 2 years ago

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

yutannihilation commented 2 years ago

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 commented 1 year ago

@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:

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 commented 1 year ago

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 commented 1 year ago

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

thomasp85 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

thomasp85 commented 1 year ago

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

yutannihilation commented 1 year ago

Oh, then, it sounds perfect to me!

teunbrand commented 1 year ago

It feels safe to close this issue

tidyverse / ggplot2

Release v3.4.0 #4854

4921 also looks nice to include in the release. But I think it's safe to have this in a patch release, so this is just a nice to have.