Skip to content

Fix imshow to work with subclasses of ndarray. #18286

Merged
dstansby merged 3 commits intomatplotlib:masterfrom
l-johnston:issue_18077
Aug 22, 2020
Merged

Fix imshow to work with subclasses of ndarray. #18286
dstansby merged 3 commits intomatplotlib:masterfrom
l-johnston:issue_18077

Conversation

@l-johnston
Copy link
Copy Markdown
Contributor

@l-johnston l-johnston commented Aug 18, 2020 •
edited
Loading

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

jklymak
jklymak approved these changes Aug 18, 2020
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

l-johnston commented Aug 18, 2020

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
Copy Markdown
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
Copy Markdown
Contributor Author

l-johnston commented Aug 18, 2020

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
Copy Markdown
Contributor

dopplershift commented Aug 18, 2020 •
edited
Loading

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 l-johnston requested a review from jklymak August 20, 2020 11:23
@l-johnston
Copy link
Copy Markdown
Contributor Author

l-johnston commented Aug 21, 2020

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

dopplershift
dopplershift approved these changes Aug 21, 2020
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

dstansby commented Aug 22, 2020

Thoughts on backporting to 3.3.x as a bugfix?

@neutrinoceros
Copy link
Copy Markdown
Contributor

neutrinoceros commented Aug 22, 2020

Thanks for fixing this !

@l-johnston l-johnston deleted the issue_18077 branch August 22, 2020 12:50
@pllim pllim mentioned this pull request Jan 22, 2024
Open
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dopplershift dopplershift dopplershift approved these changes

@dstansby dstansby dstansby approved these changes

@jklymak jklymak Awaiting requested review from jklymak

Assignees

No one assigned

Labels

API: consistency

Projects

None yet

Milestone

v3.4.0

Development

Successfully merging this pull request may close these issues.

Imshow breaks if given a unyt_array input

5 participants

@l-johnston @jklymak @dopplershift @dstansby @neutrinoceros