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: add support for varying colors in quiver annotations #3822

Merged
merged 2 commits into from
Mar 2, 2022

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented Feb 21, 2022

PR Summary

Improve wrapping around matplotlib.axes.Axes.quiver's complicated signature to enable the optional 5th positional argument C.

I'm also refactoring QuiverCallback and CuttingQuiverCallback to avoid duplicating code as much as possible.

A 'bonus' change here is that I'm allowing arbitary keyword arguments to be passed down to the wrapped method, because it's much more intuitive to pass cmap="inferno" than plot_args=dict(cmap="inferno"). If requested I can remove this from the PR and possibly repurpose it as a new PR, making similar changes to all callbacks that allow it (i.e. the ones that have only one such dictionary argument)

Another bonus is that I fixed a small bug with the normalize bool flag where users would get a divide-by-zero warning when using normalisation for a field that has 0-len elements.

usage

import yt

ds = yt.load_sample("IsolatedGalaxy")
p = yt.ProjectionPlot(ds, "z", "density")

p.annotate_quiver(
    "velocity_x",
    "velocity_y",
    "vorticity_z",
    normalize=True,
    cmap="inferno_r",
)
p.zoom(40)
p.save('/tmp/')

galaxy0030_Projection_z_density

note that this works with matplotlib 2.2 (oldest supported) as well as 3.5 (newest).
see their old doc here https://matplotlib.org/2.2.2/api/_as_gen/matplotlib.axes.Axes.quiver.html?highlight=quiver#matplotlib.axes.Axes.quiver

for completness, introducing this new "field_c" parameter is technically a breaking change for any user code relying on "factor" being usable as a third positional argument. I personally think it's okay, as long as it's explicitly mentioned in the release notes, because it's impossible to mutate the signature continuously without a breaking change. I'm making all arguments keyword-only to remedy this source of problems in the future.

@neutrinoceros neutrinoceros added bug enhancement Making something better deprecation deprecate features or remove deprecated ones labels Feb 21, 2022
@neutrinoceros neutrinoceros added this to the 4.1.0 milestone Feb 21, 2022
@neutrinoceros neutrinoceros added backwards incompatible This change will change behavior and removed deprecation deprecate features or remove deprecated ones labels Feb 21, 2022
@neutrinoceros neutrinoceros changed the title ENH: add support for colored quiver callback ENH: add support for vaying colors in quiver annotations Feb 21, 2022
@neutrinoceros
Copy link
Member Author

neutrinoceros commented Feb 21, 2022

Got CI green, but more work needed to allow usage of arbitrary color norms.
edit: never mind this already works using matplotlib.axes.Axes.quiver's norm keyword argument.

@neutrinoceros
Copy link
Member Author

I also would like expose a simple API to associate a smaller colorbar, essentially looking like this:
galaxy0030_Projection_z_density

but doing this correctly necessitates (and gives me a pretext for) a deeper refactor of how colorbars and axis labels work (essentially we need to centralise some bits of code to make them reusable).
This is definitely possible and I don't think that I'm closing any door now if I say it's out of scope for the present PR.

@neutrinoceros neutrinoceros changed the title ENH: add support for vaying colors in quiver annotations ENH: add support for varying colors in quiver annotations Feb 21, 2022
@neutrinoceros neutrinoceros marked this pull request as ready for review February 21, 2022 20:35
@matthewturk matthewturk merged commit ff5e808 into yt-project:main Mar 2, 2022
@neutrinoceros neutrinoceros deleted the colored_quivers branch March 2, 2022 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards incompatible This change will change behavior bug enhancement Making something better viz: 2D
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants