Fixed imsave() saving incorrect color map #29203
Conversation
There was a problem hiding this comment.
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 us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code 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.
This is a very confusing import in this file... this file IS matplotlib.image
You should be able to remove the mimage. below, as AxesImage is defined in this file and therefore exists in the namespace already.
There was a problem hiding this comment.
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.
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]] or np.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 than arr = np.asarray(arr, dtype=np.uint8) in an appropriate place.
Additionally, note if 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.
I think we can get away with only doing arr = np.asarray(arr) here and dropping the isinstance 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.
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.
Also arr = 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. | |||
| """ | |||
|
|
|||
| @@ -1093,6 +1094,7 @@ def set_data(self, x, y, A): | |||
| """ | |||
| Set the grid for the pixel centers, and the pixel values. | |||
|
|
|||
|
|
|||
|
@gpxxlt Do you plan to continue with this? |
|
This was replaced by #29931. |
PR summary
Goal
This pull request is made to address issue #29183. It closes #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