Skip to content

FIX: resolution of imshow for floats and 2-D greyscale PIL images #10613

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Mar 3, 2018
Merged

FIX: resolution of imshow for floats and 2-D greyscale PIL images #10613

merged 5 commits into from
Mar 3, 2018

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Feb 26, 2018 •
edited
Loading

PR Summary

Addresses big values in floating point arrays losing precision (#10567)

We discussed on the call including a warning. But I'm still not sure why this is required because this doesn't touch the original data, just the scaled data (before it is scaled).

New test is added. Before the change the test code would yield:

    fig, ax = plt.subplots()
    img = np.array([[1, 2, 1e22],[3, 1, 4]], dtype=np.uint64)
    pc = ax.imshow(img)
    pc.set_clim(0, 5)
    plt.show()

figure_1-3old

After:

figure_1-3new

EDIT: Added a fix for #10616 when an image from PIL is 2-D this converts to RGBA in set_dataso it is rendered in greyscale.

This needs a test. Also needs some approval that this is the correct route. @efiring thought we should just cast to a 2-D masked array, but that will go through the norm and colormap route, and would therefore not plot as greyscale.

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant

@jklymak jklymak mentioned this pull request Feb 26, 2018
Closed
dopplershift
dopplershift reviewed Feb 26, 2018
View reviewed changes
lib/matplotlib/image.py Outdated
if newmin > a_min:
A_scaled[A_scaled < newmin ] = newmin
a_min = np.float64(newmin)
newmax = vmid + dv * 1.e7
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these newmax/newmin appropriate if LogNorm or something similarly non-linear is being used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Grrrrr, not sure. Probably not!

Of course, the way the interpolation is done is questionable if you use LogNorm. Right now we are doing the interpolation in data space instead of normed-space. It seems the latter is more appropriate for non-linear norms. ping @tacaswell

Copy link
Member

Choose a reason for hiding this comment

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

can we use np.clip here?

Do we actually have to do this indexing step or can we reset a_min and a_max and let Agg deal with the clipping to [0, 1] internally?

Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear, I'm not saying we shouldn't take your approach--just wondering if a check is in order.

Copy link
Member Author

Choose a reason for hiding this comment

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

In LogNorm vmin and vmax are in data space, not the log of the data, so it should work fine for LogNorm. Not sure about all the other norms.

Copy link
Member Author

Choose a reason for hiding this comment

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

The norms all work with vmin and vmax in data space (rather than transformed space), so this fix is fine. I've uploaded a version that uses np.clip instead of indexing.

@tacaswell we indeed need to clip right away - not clipping is the whole problem with the floating point math of the next step.

@tacaswell tacaswell added this to the v2.2.0 milestone Feb 26, 2018
@jklymak jklymak mentioned this pull request Feb 27, 2018
Closed
@jklymak jklymak changed the title FIX: resolution of imshow for floats.... FIX: resolution of imshow for floats and 2-D greyscale PIL images Feb 27, 2018
@jklymak jklymak added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Feb 27, 2018
efiring
efiring approved these changes Mar 3, 2018
View reviewed changes
Copy link
Member

@efiring efiring left a comment

Choose a reason for hiding this comment

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

Both parts of this look good to me. My initial suggestion regarding handling the jpeg problem was clearly wrong, as I suspected it might be.

@tacaswell tacaswell merged commit 6eef1c8 into matplotlib:master Mar 3, 2018
@tacaswell
Copy link
Member

Thanks @jklymak !

@lumberbot-app lumberbot-app bot mentioned this pull request Mar 3, 2018
Merged
lumberbot-app bot pushed a commit that referenced this pull request Mar 3, 2018
@tacaswell
Backport PR #10613: FIX: resolution of imshow for floats and 2-D grey…
913f172 
…scale PIL images
tacaswell added a commit that referenced this pull request Mar 4, 2018
@tacaswell
Merge pull request #10664 from matplotlib/auto-backport-of-pr-10613
cc55f83 
Backport PR #10613 on branch v2.2.x
@jklymak jklymak mentioned this pull request Mar 8, 2018
Merged
2 tasks
@hagenw hagenw mentioned this pull request Mar 14, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tacaswell tacaswell tacaswell left review comments

@dopplershift dopplershift dopplershift left review comments

@efiring efiring efiring approved these changes

Assignees
No one assigned
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Milestone
v2.2.0
Development

Successfully merging this pull request may close these issues.
4 participants
@jklymak @tacaswell @efiring @dopplershift