-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Add support for images with units #27721
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if this is scalarmappables than that I think should be the title, but I'm a bit confused about what's happening w/ categorical here especially given scatter categorical color is a pretty big use case for extending units to scalarmappable. Not that this PR has to do that, but I'm concerned about this PR preventing it.
lib/matplotlib/category.py
Outdated
def _check_axis(axis): | ||
if axis is None: | ||
raise units.ConversionError( | ||
"Categorical data does not support unit conversion that is not " | ||
"attached to an x or y Axis." | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh? Does this mean no categorical color? Technically, categorical should work just fine w/ NoNorm (and a colorbar where the labels are in the middle rather than the edges).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means no categorical color in this PR, because I didn't dive into the error it gave me... you raise a good point though that this PR should as a minimum not prevent categorical colorbars in the future. Let me take a look again at some point later this week, and I'll ping you with what I find.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the error? Just curious if it would be the same issue for #27706
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was
RuntimeError: <matplotlib.category.StrCategoryConverter object at 0x126bcd610> failed when trying to return the default units for this image. This may be because support has not been implemented for `axis=None` in the default_units() method.
because StrCategoryConverter.default_units()
actually uses the axis
argument (unlike all our other conversion interfaces.
I think this is fixable though, not sure why I just put those errors in... Don't have time now, but will chase categorical support next time I come back to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, wondering if we can make a formatter for this, something gets to: (b/c is also useful for any of the discrete cases)
fig, ax = plt.subplots()
im = ax.imshow(([[0,1,2], [2, 0, 1], [1, 2, 0]]), cmap=mcolors.ListedColormap(["tab:orange", "tab:blue", "tab:green"], 3))
cb = fig.colorbar(im)
cb.set_ticks([1/3, 1, 1+2/3])
cb.set_ticklabels([0,1,2])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think to do this as a legend would probably be via helper method that grabs the color->value mapping and generates the handles, something like https://matplotlib.org/devdocs/api/collections_api.html#matplotlib.collections.PathCollection.legend_elements but on ScalarMappable
(Which if you're cool w/ me branching off your PR, I may volunteer to do...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this is in a good enough state (== tests passing) now if you want to branch off and play around with the colorbar for categoricals. I'm not intending to change anything there myself in this PR, but would be happy to merge something into this branch/PR if it's not too complicated.
273d3f7
to
574999d
Compare
e581418
to
8904ab5
Compare
The new colormapping pipeline merged (#28658) probably means that it'd be easier to create a new PR than rebase this one, but also should I think make the actual make it support units easier. |
PR summary
This PR adds machinery for the data array given to a
ScalarMappable
to have units, and for those units to be used with images and associated colorbars.Fixes #25062
Fixes #17447
Fixes #19476
Fixes astropy/astropy#11306
Still needs a user facing example adding, but opening to get early feedback and full CI runs.
PR checklist