Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Fixed imsave() saving incorrect color map#29203
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join uson gitter for real-time discussion.
For details on testing, writing docs, and our review process, please seethe developer guide
We strive to be a welcoming and open project. Please follow ourCode of Conduct.
…ally to ensure it passes checks on github.
Hi, thanks for tackling this! Can you change the title and content to something that describes what this PR is doing? That may help attract reviewers. |
Thank you@story645 for your advice. I am currently editing this pull request and waiting for the newest check to complete. |
lib/matplotlib/image.py Outdated
| @@ -28,6 +28,8 @@ | |||
| Affine2D, BboxBase, Bbox, BboxTransform, BboxTransformTo, | |||
| IdentityTransform, TransformedBbox) | |||
| import matplotlib.image as mimage | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is a very confusing import in this file... this file ISmatplotlib.image
You should be able to remove themimage. below, asAxesImage is defined in this file and therefore exists in the namespace already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hi@ksunden, thank you for pointing out this confusing line. I have made small adjustments in the most recent update. Please let me know if it looks good. Thank you for your time!
lib/matplotlib/image.py Outdated
| # This specifically handled list-of-list-of-list | ||
| # Produced an image instance using the data from arr and do scaling | ||
| if (isinstance(arr, list)): | ||
| fig = Figure() | ||
| ax = fig.add_axes([0, 0, 1, 1], | ||
| aspect='auto', | ||
| frameon=False, | ||
| xticks=[], | ||
| yticks=[]) | ||
| im = AxesImage(ax, cmap=cmap) | ||
| im.set_data(arr) | ||
| im._scale_norm(None, vmin, vmax) | ||
| arr = im.get_array() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm confused: Why do we explicitly scale if the data are lists and not if they are arrays? Shouldn't they be handled equally wrt to scaling whether it's[[1, 2]], [3, 4]] ornp.array([[1, 2]], [3, 4]])? Also, are we sure that this plays well with the following if/else block? Overall, from#29183 (comment) I would have expected nut much more thanarr = np.asarray(arr, dtype=np.uint8) in an appropriate place.
Additionally, noteif separate scaling is needed, it should look more like the scaling in the else block below and not create a whole figure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we can get away with only doingarr = np.asarray(arr) here and dropping theisinstance check in the next line.
Please correct your Git config; it is using the username of your computer at the temporary DNS name of your computer for the email address. Neither |
| if (isinstance(arr, list)): | ||
| arr = np.asarray(arr, dtype=np.uint8) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Let's move the format coersion above the origin handling. While the reversal ([::-1]) also works for lists, it's more efficient and easier to understand if we ensure that we have the array already there.
Alsoarr = np.asanyarray(array) should be enough.
asanyarray()instead ofasarray()Passing through array subclasses should be ok - we currently do that as well.- let's not do dtype conversion in this PR - we currently also don't do this for arrays. The user is responsible for passing reasonable types. Any change to that would be a separate topic.
| @@ -5869,6 +5869,7 @@ def imshow(self, X, cmap=None, norm=None, *, aspect=None, | |||
| `~matplotlib.pyplot.imshow` expects RGB images adopting the straight | |||
| (unassociated) alpha representation. | |||
| """ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| @@ -1093,6 +1094,7 @@ def set_data(self, x, y, A): | |||
| """ | |||
| Set the grid for the pixel centers, and the pixel values. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@gpxxlt Do you plan to continue with this? |
This was replaced by#29931. |
Uh oh!
There was an error while loading.Please reload this page.
PR summary
Goal
This pull request is made to address issue#29183. Itcloses#29183.
Implementation
This pull request added an
ifbranch within the functionimsave()in the pathlib/matplotlib/image.py. It is added to handle the case addressed in issue#29183 where inputarris a vanilla python list.Testing
One test case is added to
lib/matplotlib/tests/test_image.pyso there is a round trip fromimread()toimsave(). Test input is designed to be a vanilla python list so that the newly added branch is covered in test suite.PR checklist