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

The future of update_geom_defaults() #6085

Open
teunbrand opened this issue Sep 6, 2024 · 7 comments · Fixed by #6158
Open

The future of update_geom_defaults() #6085

teunbrand opened this issue Sep 6, 2024 · 7 comments · Fixed by #6158

Comments

@teunbrand
Copy link
Collaborator

teunbrand commented Sep 6, 2024

This issue came up in #6084 (comment).

Briefly, now that we can do theme(geom = element_geom(...)), there is much less need for update_geom_defaults(). Per #6084 (comment), we should ask ourselves if update_geom_defaults() should be deprecated in a future release.

In my opinion, there is still a niche for update_geom_defaults() as one might want to swap out defaults anyway, in particular when it should affect 1 particular geom and not all of them. I sometimes (ab)use it to get geoms to accept the aesthetics I want (example). I'd therefore advise against future deprecation, but we should probably pivot from recommending update_geom_defaults() to theme(geom).

@yutannihilation
Copy link
Member

Sorry, it might be a bit confusing that I use the term "soft-deprecated" in #6084. I didn't mean to formally deprecate it. Let me clarify a bit.

In my understanding, now the recommendation is

  1. want to change a parameter of a specific geom of a specific plot -> specify the parameter in geom_*() directly
  2. want to change a parameter of all geoms of a specific plot -> use theme(geom = element_geom(...))
  3. want to change a parameter of a specific geom of all plots -> use update_geom_defaults()
  4. want to change a parameter of all geoms of all plots -> use theme_update(geom = element_geom(...)) (or theme_set()?)

and there's still no alternative for case 3. So, update_geom_defaults() needs to be kept. What I meant was, since the usage looks like rather for exports, the documentation for ordinary users should recommend geom = element_geom() over update_geom_defaults() in general. But, while saying this is easy, deciding the nuance is probably subtle and difficult. Honestly, I don't know what to do...

@teunbrand
Copy link
Collaborator Author

teunbrand commented Sep 6, 2024

Thanks for the clarification, I had thought you meant lifecycle::deprecate_soft() but it is clearer now :)
If we agree that there are niche use cases but generally users should opt for theme(geom), perhaps it makes sense to mark update_geom_defaults() as superseded? I think that'd send the right message of 'hey, it is still here, but we'd prefer you to use something else'.
I think your recommendations would make a fine section for a future release post

@yutannihilation
Copy link
Member

perhaps it makes sense to mark update_geom_defaults() as superseded?

In my understanding, "superseded" is the status that all usages are superseded by other functions. So, as long as it still has its unique use, probably it's not superseded yet. But, it's true that some of the usages are superseded. It's difficult to describe the nuance precisely...

I think your recommendations would make a fine section for a future release post

Hope it helps.

@teunbrand
Copy link
Collaborator Author

I'm reopening this issue in relation to #6285. Essentially, that PR would make update_geom_defaults() obsolete.
However, I also realised that migrating from ggplot2 3.5.0 to 3.6.0 as an extension package might necessitate the use of update_geom_defaults().

Imagine you're the author of Geom extension and you want to (1) use the new theme-based defaults but also (2) not break your package when an older ggplot2 version is loaded.

To me, one reasonable solution I can come up with is to use update_geom_defaults() in a conditional on_load() expression to bump the defaults if the new ggplot2 version is loaded. In this case we can't quite retire the update_geom_defaults() function yet.

@clauswilke
Copy link
Member

@teunbrand Deleted a comment that I wrote because I think I'm not fully caught up to all the features you have already implemented. :-)

Either way, my point was that it's really important to have long times where two features overlap, the new and the old, so that there is plenty of time for extension developers to update their package. The worst is when you basically have to release your new package the day the new ggplot2 gets released but can't release even one day earlier.

Finally, getting rid of update_geom_defaults() might deserve a version bump to 4.0.

@teunbrand
Copy link
Collaborator Author

I think I'm not fully caught up to all the features you have already implemented

Admittedly, the news file has accumulated a bulky section for the upcoming update :)

Either way, my point was that it's really important to have long times where two features overlap, the new and the old, so that there is plenty of time for extension developers to update their package.

update_geom_defaults() can harmoniously co-exist with the new system, but it is just being (ab)used in all sorts of different contexts. I think the best thing would probably be to set the deprecation cycle in motion and just start throwing warnings to developers, before starting to throw them to users (the soft deprecation thing). I'm happy to postpone this a release cycle if you think that is wise.

@clauswilke
Copy link
Member

I would say if the new system that pulls defaults from the theme hasn't even been released yet in an official CRAN release then don't deprecate the old version yet. Just let them coexist for a release cycle or two. When you're the one developing the new features it can often feel like a feature has been in the codebase forever and it's time to deprecate the old approach but the vast majority of the outside world has never even seen the new approach at this time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants