Skip to content

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

Merged
tacaswell merged 5 commits into
matplotlib:masterfrom
jklymak:fix-color-resolution
Mar 3, 2018
Merged

FIX: resolution of imshow for floats and 2-D greyscale PIL images#10613
tacaswell merged 5 commits into
matplotlib:masterfrom
jklymak:fix-color-resolution

Conversation

@jklymak

@jklymak jklymak commented Feb 26, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

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
Comment thread lib/matplotlib/image.py
if newmin > a_min:
A_scaled[A_scaled < newmin ] = newmin
a_min = np.float64(newmin)
newmax = vmid + dv * 1.e7

@dopplershift dopplershift Feb 26, 2018

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.

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

@jklymak jklymak Feb 26, 2018

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

@tacaswell tacaswell Feb 26, 2018

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.

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?

@dopplershift dopplershift Feb 26, 2018

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.

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

@jklymak jklymak Feb 26, 2018

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

@jklymak jklymak Feb 26, 2018

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

@efiring efiring 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.

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

tacaswell commented Mar 3, 2018

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