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

Fix imshow to work with subclasses of ndarray. #18286

Merged
merged 3 commits into from Aug 22, 2020
Merged

Fix imshow to work with subclasses of ndarray. #18286

merged 3 commits into from Aug 22, 2020

Conversation

l-johnston
Copy link
Contributor

@l-johnston l-johnston commented Aug 18, 2020

PR Summary

Closes #18077

In the internals of _ImageBase._make_image, the clip bounds arithmetic fails with ndarray subclasses that support masked arrays such as unyt.

PR Checklist

  • Code is Flake 8 compliant
  • Code has test coverage

Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

Seems right - OTOH do you have a test?

@jklymak jklymak added this to the v3.4.0 milestone Aug 18, 2020
@l-johnston
Copy link
Contributor Author

I took a look at matplotlib's Quantity defined in test_units.py, but it doesn't have all the ndarray features unyt has that are needed to test the fix. And I didn't want to import unyt.

@jklymak jklymak changed the title Fix imshow to work with subclasses of ndarray. Fix #18077 Fix imshow to work with subclasses of ndarray. Aug 18, 2020
@jklymak
Copy link
Member

jklymak commented Aug 18, 2020

(Please don't put the issue in title, but yes, please do put issue in description!)

@l-johnston
Copy link
Contributor Author

I might be able to implement a skeleton ndarray subclass with just enough features to reproduce the bug and then test the fix. Is this of interest?

@dopplershift
Copy link
Contributor

dopplershift commented Aug 18, 2020

If it's a huge amount of effort, I think it would be worthwhile. That should have said: "If it's NOT a huge amount of effort, I think it would be worthwhile."

@l-johnston
Copy link
Contributor Author

@dopplershift I implemented the skeleton ndarray subclass that reproduces the bug and validates the fix. Is this sufficient?

Copy link
Contributor

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

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

That seems fine to me.

There is a slight question as to whether this should go into test_units.py (see #18278), but I'm not holding this up for that.

@dstansby dstansby merged commit 48b6327 into matplotlib:master Aug 22, 2020
@dstansby
Copy link
Member

Thoughts on backporting to 3.3.x as a bugfix?

@neutrinoceros
Copy link
Contributor

Thanks for fixing this !

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

Successfully merging this pull request may close these issues.

Imshow breaks if given a unyt_array input
5 participants