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

ENH: avoid implicitly dropping default parameters in plot annotation methods #4475

Merged

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented Jun 6, 2023

PR Summary

Many plot annotation methods have 'hidden' internal defaults parameters that may be overriden by users, but passing a single argument (or even an empty dict plot_args={}) results in dropping all default parameters.

For example

import yt
ds = yt.load_sample("IsolatedGalaxy")
p = yt.SlicePlot(ds, "z", ("gas", "density"))
p.annotate_timestamp()
p.save("/tmp/1.png")

1

p.clear_annotations()
p.annotate_timestamp(text_args={"horizontalalignment": "left"})
p.save("/tmp/2.png")

2
(user wants to override horizontalalignement, but the colour is also changed !)

In methods that have more than a couple default parameters, this means that the slightest mutation from default behaviour may require a large effort from the user just to replicate the internal defaults.
This doesn't have to be so: we can instead compose parameter sets from internal defaults and user-provided ones.

This is an attempt to implement this composition interface across all plot annotation methods.
Special care is needed to test this because matplotlib may emit warnings about unused parameters with certain combinations. I'll leave this as a draft until I'm confident this is handled properly.

@neutrinoceros neutrinoceros added enhancement Making something better viz: 2D UX user-experience labels Jun 6, 2023
@neutrinoceros neutrinoceros marked this pull request as draft June 6, 2023 09:52
@neutrinoceros neutrinoceros force-pushed the no_implicit_full_override branch from f4e4cbb to 6cbcc7c Compare June 6, 2023 16:24
@neutrinoceros neutrinoceros added the api-consistency naming conventions, code deduplication, informative error messages, code smells... label Jun 6, 2023
@neutrinoceros neutrinoceros force-pushed the no_implicit_full_override branch from 6cbcc7c to 9929fa7 Compare June 6, 2023 16:56
@neutrinoceros neutrinoceros linked an issue Jun 6, 2023 that may be closed by this pull request
@neutrinoceros
Copy link
Member Author

Now fixes #4459.
Explanation: to my knowledge, ContourCallback is the only chunk of code in yt that cares about contour parameters.
Now, adding negative_linestyles="solid" to the internal default parameters of this method is tricky, because it's only used if linestyle=None, and matplotlib will emit warnings about unused parameters, and we want to avoid that. I think this was the original motivation for mutating matplotlib's config at runtime instead.
With this PR, linestyles="solid" is always passed down to matplotlib's contour method (unless overridden explicitly by the user), so nothing else is required to make negative contours use solid line too.

@neutrinoceros neutrinoceros added this to the 4.3.0 milestone Jun 6, 2023
@neutrinoceros
Copy link
Member Author

neutrinoceros commented Jun 6, 2023

I'm not sure this PR qualifies as a bugfix as a whole, because it isn't meant to be 100% backward compatible
and in any case it's not urgent to backport, so for now I'm aiming it at the next feature release.

@neutrinoceros neutrinoceros force-pushed the no_implicit_full_override branch from 9929fa7 to b28b559 Compare June 6, 2023 18:29
@neutrinoceros neutrinoceros marked this pull request as ready for review June 6, 2023 19:20
@matthewturk
Copy link
Member

I think I've changed my mind. This is a bugfix. And, it fixes a pretty important set of behavior.

@matthewturk matthewturk enabled auto-merge June 9, 2023 17:00
Copy link
Member

@matthewturk matthewturk left a comment

Choose a reason for hiding this comment

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

I do really like **(plot_args or {}) for some reason.

@matthewturk matthewturk merged commit c9eab3a into yt-project:main Jun 9, 2023
@neutrinoceros neutrinoceros deleted the no_implicit_full_override branch June 9, 2023 17:02
@neutrinoceros neutrinoceros modified the milestones: 4.3.0, 4.2.1 Jun 9, 2023
@neutrinoceros
Copy link
Member Author

alright let's backport this bad boy then

@meeseeksdev backport to yt-4.2.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-consistency naming conventions, code deduplication, informative error messages, code smells... bug enhancement Making something better UX user-experience viz: 2D
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: yt may leak matplotlib configuration globally at runtime
2 participants