Skip to content

Fix imshow to work with subclasses of ndarray. #18286

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

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

Conversation

@l-johnston

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

Copy link
Copy Markdown
Contributor

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

@jklymak jklymak left a comment

Copy link
Copy Markdown
Member

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
@jklymak jklymak added the API: consistency Consistency of the matplotlib API, including naming, behavior, defaults, … label Aug 18, 2020
@l-johnston

l-johnston commented Aug 18, 2020

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

jklymak commented Aug 18, 2020

Copy link
Copy Markdown
Member

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

@l-johnston

l-johnston commented Aug 18, 2020

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

dopplershift commented Aug 18, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

l-johnston commented Aug 21, 2020

Copy link
Copy Markdown
Contributor Author

@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

@dopplershift dopplershift left a comment

Copy link
Copy Markdown
Contributor

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

dstansby commented Aug 22, 2020

Copy link
Copy Markdown
Member

Thoughts on backporting to 3.3.x as a bugfix?

@neutrinoceros

neutrinoceros commented Aug 22, 2020

Copy link
Copy Markdown
Contributor

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 Consistency of the matplotlib API, including naming, behavior, defaults, …

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