Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Fixed an off-by-half-pixel bug in image resampling when using a nonaffine transform (e.g., a log axis)#30054

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

Open
ayshih wants to merge2 commits intomatplotlib:main
base:main
Choose a base branch
Loading
fromayshih:resample_bug

Conversation

ayshih
Copy link
Contributor

@ayshihayshih commentedMay 14, 2025
edited
Loading

PR summary

This PR fixes a off-by-half-pixel bug inmatplotlib._image.resample() when using a nonaffine transform. (This function is used internally when drawing an image artist.) The mesh for nonaffine transforms is mistakenly computed at the lower corners of pixels instead of the centers of pixels, which results in a half-pixel shift in the output. Here are illustrative plots and the generating script.

Before this PR:
download (16)

After this PR:
Figure_1

Script:

importmatplotlib.pyplotaspltimportnumpyasnpfrommatplotlib.transformsimportAffine2D,Transformfrommatplotlib.imageimportresample,NEAREST,BILINEARin_data=np.array([[0.1,0.3,0.2]])in_shape=in_data.shapein_edges=np.arange(in_shape[1]+1)out_shape= (1,10)out_edges=np.arange(out_shape[1]+1)affine_data=np.empty(out_shape)nonaffine_data=np.empty(out_shape)# Create a simple affine transform for scaling the input arrayaffine=Affine2D().scale(sx=out_shape[1]/in_shape[1],sy=1)# Create a nonaffine version of the same transform by compositing with a nonaffine identity transformclassNonAffineIdentityTransform(Transform):input_dims=2output_dims=2definverted(self):returnselfnonaffine=NonAffineIdentityTransform()+affinefig,axs=plt.subplots(3,1,figsize=(4.8,6.4),layout="constrained")axs[0].stairs(in_data[0, :],in_edges)axs[0].grid(ls='dotted')axs[0].set_xticks(in_edges)axs[0].set_title('Original data')resample(in_data,affine_data,affine,interpolation=NEAREST)resample(in_data,nonaffine_data,nonaffine,interpolation=NEAREST)axs[1].stairs(affine_data[0, :],out_edges,label='affine')axs[1].stairs(nonaffine_data[0, :],out_edges,ls='dashed',label='nonaffine')axs[1].grid(ls='dotted')axs[1].set_xticks(out_edges)axs[1].legend()axs[1].set_title('Nearest-neighbor resampling')resample(in_data,affine_data,affine,interpolation=BILINEAR)resample(in_data,nonaffine_data,nonaffine,interpolation=BILINEAR)axs[2].stairs(affine_data[0, :],out_edges,label='affine')axs[2].stairs(nonaffine_data[0, :],out_edges,ls='dashed',label='nonaffine')axs[2].grid(ls='dotted')axs[2].set_xticks(out_edges)axs[2].legend()axs[2].set_title('Linear resampling')plt.show()

PR checklist

@ayshihayshihforce-pushed theresample_bug branch 2 times, most recently frome0d192d to6729873CompareMay 14, 2025 18:32
Copy link
Contributor

@scottshambaughscottshambaugh left a comment
edited
Loading

Choose a reason for hiding this comment

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

On small change request but otherwise looks good to me. This is a very subtle bug - I'm incredibly curious about the story of how you found it!

@ayshih
Copy link
ContributorAuthor

Indeed, hunting down this bug was an adventure! For SunPy, we do some fun stuff with remapping data to a different view, e.g.:
download (87)
We had an implementation that usedpcolormesh(), but we recently added an alternative option to tryimshow() instead. That means supplyingimshow() with a nonaffine transform, which meant that it was affected by the bug fixed in this PR. There were peculiar differences between the outputs of the two approaches, but always no larger than a display pixel, so I wasn't sure whether I was crazy or not. Because it's at the display-pixel level, you can't simply zoom in interactively on a plot to see the discrepancy more clearly. It was only after I found thatimshow() with justtransData and with a nonaffine identity transform composited withtransData gave different results that I started peeling through all of the layers in matplotlib to find the bug.

rcomer reacted with thumbs up emojircomer and scottshambaugh reacted with rocket emojircomer reacted with eyes emoji

@oscargusoscargus mentioned this pull requestMay 16, 2025
@oscargus
Copy link
Member

I added a stub file in#30058 to have the mypy errors go away.

It is slightly preferred if you can create a test using the public API (not accessing the_image module directly), but I leave it up to you if you rewrite the tests or wait for#30058. Otherwise looks good! Nice catch!

@ayshih
Copy link
ContributorAuthor

ayshih commentedMay 16, 2025
edited
Loading

It is slightly preferred if you can create a test using the public API (not accessing the_image module directly)

Heh, I was hesitant to access_image in a test until I noticed that an existing test already did so, which is why I put this new test right below that one. I think it is prudent to keep a test that accesses_image module directly, given that it is a C extension and it's helpful to know if something breaks in the extension rather than in the Python above it.

There are a bunch of layers of private API between_image.resample() and public API:_image.resample() ->image._resample() ->image._ImageBase._make_image() ->image.*Base.make_image(). So, not only would the test be a little awkward to write, a future developer would have to repeat what I did in peeling back all of the Python layers before realizing that the relevant code is actually in the C extension.

(Edit: As realized down below,_image.resample() is actually already imported asimage.resample(), so it is straightforward to convert the test to public API.)

I'll point out that this PR implicitly already has a test using public API, given that baseline images had to be updated. The plotting of an image using a log scale uses a nonaffine transform, and hence was affected by the bug.

Comment on lines 1645 to 1647
[(np.array([[0.1, 0.3, 0.2]]), mpl._image.NEAREST,
np.array([[0.1, 0.1, 0.1, 0.3, 0.3, 0.3, 0.3, 0.2, 0.2, 0.2]])),
(np.array([[0.1, 0.3, 0.2]]), mpl._image.BILINEAR,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[(np.array([[0.1,0.3,0.2]]),mpl._image.NEAREST,
np.array([[0.1,0.1,0.1,0.3,0.3,0.3,0.3,0.2,0.2,0.2]])),
(np.array([[0.1,0.3,0.2]]),mpl._image.BILINEAR,
[(np.array([[0.1,0.3,0.2]]),mimage.NEAREST,
np.array([[0.1,0.1,0.1,0.3,0.3,0.3,0.3,0.2,0.2,0.2]])),
(np.array([[0.1,0.3,0.2]]),mimage.BILINEAR,

These values are implemented in a private namespace, but are available in a public namespace (which is already imported directly)

This should also resolve the type hint problems

@oscargus
Copy link
Member

Fair enough. As I said, it is preferred but not strictly required. More that is was worth asking, especially with the mypy errors (for which my solution did not work btw, but the one provided above should).

@ayshih
Copy link
ContributorAuthor

ayshih commentedMay 16, 2025
edited
Loading

@ksunden Thanks for pointing out thatimage hasfrom matplotlib._image import *! Thus, not only are the constants imported into theimage namespace, but so is_image.resample() itself. So, I have adjusted the test to use entirely public API.

@QuLogic
Copy link
Member

QuLogic commentedMay 20, 2025
edited
Loading

I can mostly understand the line plots, but do you have an example of how this changes in an image itself? Possibly one that is rather low resolution so it is clear how the changes affect it. The test image that did change is a bit too high resolution to see clearly, I think.

@ayshih
Copy link
ContributorAuthor

As I mentioned above, it's a little challenging to show the behavior in an obvious manner because it is typically only at the display-pixel level. That means you can't see the discrepancies better through zooming in an interactive plot or plotting at a larger DPI. Perhaps there is some way I am not aware of, but I take a different approach below.

Here's a comparison script to generate images for comparison (without the fix in this PR):

importnumpyasnpimportmatplotlib.pyplotaspltfrommatplotlib.transformsimportTransformdata=np.arange(12321).reshape((111,111))%4classNonAffineIdentityTransform(Transform):input_dims=2output_dims=2definverted(self):returnselffig=plt.figure()ax=fig.add_subplot()ax.imshow(data,extent=[0,111,0,111],transform=ax.transData)ax.set_title('affine')fig.savefig('affine.png')fig=plt.figure()ax=fig.add_subplot()ax.imshow(data,extent=[0,111,0,111],transform=NonAffineIdentityTransform()+ax.transData)ax.set_title('nonaffine')fig.savefig('nonaffine.png')

At normal display, it's difficult to discern any difference between the two images:
affine
nonaffine

If I then use an image editor to zoom in to the origin:

affine:
affine_zoom
nonaffine:
nonaffine_zoom

The data array was specifically chosen so that 3 data pixels map to 10 display pixels, just as with the line plots above. You can see that between the affine image and the nonaffine image, the 3-4-3 pattern of display pixels has shifted. (That is, a data pixel that used to span 3 display pixels now spans 4 display pixels, and the data pixel right after it has changed from 4 display pixels to 3 display pixels.) It is difficult to know which one is "right" from these images, given all of the additional stuff that is done to render the image, but that's why the test in this PR checks the output of the low-levelresample().

@ayshihayshih changed the titleFixed an off-by-half-pixel bug in image resampling when using a nonaffine transformFixed an off-by-half-pixel bug in image resampling when using a nonaffine transform (e.g., a log axis)May 27, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@ksundenksundenksunden left review comments

@scottshambaughscottshambaughscottshambaugh approved these changes

@dstansbydstansbydstansby approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

6 participants
@ayshih@oscargus@QuLogic@ksunden@dstansby@scottshambaugh

[8]ページ先頭

©2009-2025 Movatter.jp