-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Fixed several accuracy bugs with image resampling #30184
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| if(i <= pivot) m_weight_array[pivot - i] = value; | ||
| } | ||
| unsigned end = (diameter() << image_subpixel_shift) - 1; | ||
| m_weight_array[0] = m_weight_array[end]; |
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 line is one of the bugs. 0 and end are different distances from pivot, so the corresponding weights typically should not be the same.
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.
Please put the new version under a macro guard, as was done in #28122, so that we keep a track of what's the original Agg and what we changed on top of it.
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.
Makes sense, done
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.
It turns out that I can't add a preprocessor definition in _image_resample.h a la #28122. There is buggy code in agg_image_filters.cpp that needs to be skipped, but since it is a CPP file rather than a H file, it gets compiled separately with no awareness of _image_resample.h. I have put the preprocessor definition (MPL_FIX_IMAGE_FILTER_LUT_BUGS) here at the top of agg_image_filters.h, which is lower level than would be nice, but at least still makes it clear what is custom code.
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.
You could also put the define in the corresponding meson.build if you prefer.
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.
Ah, that's a nicer approach. Done!
You can see such a blip in the example above, but it's easy to overlook. Here's a starker example: an array of two pixels with the same value (0.1) is resampled to an array of ten pixels. Before this PR, two of the pixels in the output array have values slightly greater than 0.1. >>> import numpy as np
>>> from matplotlib.transforms import Affine2D
>>> from matplotlib.image import resample, BILINEAR
>>>
>>> in_data = np.array([[0.1, 0.1]])
>>> in_shape = in_data.shape
>>>
>>> out_shape = (1, 10)
>>>
>>> # Create a simple affine transform for scaling the input array
>>> affine = Affine2D().scale(sx=out_shape[1] / in_shape[1], sy=1)
>>>
>>> affine_data = np.empty(out_shape)
>>> resample(in_data, affine_data, affine, interpolation=BILINEAR)
>>>
>>> print(affine_data)
[[0.1 0.10001221 0.1 0.1 0.1 0.1
0.10001221 0.1 0.1 0.1 ]]After this PR, the output is the expected: >>> print(affine_data)
[[0.1 0.1 0.1 0.1 0.1 0.1 0.1 0.1 0.1 0.1]] |
d2fa1e5 to
7eb18b2
Compare
7eb18b2 to
d9f8bb5
Compare
|
This killed the log-scale images? Also this changed a bunch of image tests, but I can't see any difference by eye. Is it worth just relaxing the tolerance on those tests a bit and at the same time adding new tests for these fixes? |
7eb1cea to
5cbb7bb
Compare
Indeed, I need to track down why.
Yes, the other 17 image tests are mostly subtle differences, but merely relaxing the tolerance doesn't make sense to me. The updated baseline images do represent what the output should be. |
|
Fair enough - however, it would be good if there were some tests devised where it showed a visual effect of these inaccuracies. |
e198af6 to
717a221
Compare
| } | ||
| } | ||
|
|
||
| #ifndef MPL_FIX_IMAGE_FILTER_LUT_BUGS |
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.
The following code block is removed because it not only has the same bugs as the code in agg_image_filters.h, but also it shouldn't even exist because it modifies the weight array after it has been normalized, thus potentially destroying the normalization.
43b912c to
90bd95f
Compare
|
I fixed the issue with On the other hand, most of the updated images have solely very slight changes in the appearance of text. As @jklymak suggested, it might make more sense in those cases to simply increase the tolerance instead for the comparison instead of updating the baseline image, so let me know if I should do that instead. |
|
Are you sure you have the right set of images? |
|
OK, I see that |
90bd95f to
9fdca21
Compare
0e0e2a1 to
c6dfae2
Compare
c6dfae2 to
44b9faa
Compare
|
Since you're deep into this code right now, I wonder if you have any ideas about #14143? |
Thanks for pointing out that issue! (and #1441 before it) It's all tied to the subpixel architecture of Agg, so some level of discrepancy is unavoidable given that architecture. I'll look into whether there's any bug that's making the discrepancies larger than they need to be. |
5488307 to
19ca801
Compare
I think you missed the fact that it's not the whole weight array that is normalized, but rather each set of corresponding subpixels. That is, there are 256 normalization constraints. For example, a weight array The system of equations is very annoying, and there may not be an efficient way to get to an acceptable solution even when one exists. Also, there'd be some question of whether negative weight values are allowed if the original weight array does not have any. Given the above, I feel that it is justifiable for matplotlib to avoid this headache by accepting a tiny bit of asymmetry in the weight array. |
|
I've done a deep dive into the subpixel architecture of Agg, and it's a bit of an inconsistent mess. There are 256 subpixels per pixel. Most of the Agg code treats the subpixels as if they are "center-aligned" (by this, I mean that the center of subpixel 128 is coincident with the center of the original pixel), but some of the Agg code – specific to affine transformations – treats the subpixels as if they are "edge-aligned" (by this, I mean that left edge of subpixel 0 is coincident with the left edge of the original pixel).
For nonaffine transformations, matplotlib provides a distortion table to Agg, . So, 72d759a now aligns the subpixels of the distortion table as appropriate for the interpolation type. With this change, the nonaffine analogues to #1441 and #14143 (which use nearest-neighbor interpolation) look correct, while maintaining the better behavior earlier in this PR for other types of interpolation. For affine transformations, the mix of center-aligned code and edge-aligned code in Agg can be fixed, but it requires a bunch of modifications and API changes. Even with those changes, plots like #1441 and #14143 still do not come out correct because of the rounding-to-subpixel code that I described above (#30184 (comment)). That is, there will always be the potential for misalignments up to a subpixel (1/256 of a pixel) with affine transformations. So, rather than making all of those changes to Agg, I decided that the more sensible approach for affine transformations is to first check how much the transformation scales up the image (46e0171) prior to using Agg. If the scale factor is at least 256 / 2 = 128, then there can be subpixel-related artifacts in the output image. In that case, this PR redirects the transformation down the code path for nonaffine transformations and passes it to Agg as such. With this change, #1441 and #14143 are now fixed: |
19ca801 to
e894604
Compare
e894604 to
abe59d9
Compare
abe59d9 to
c9c6c0e
Compare
|
I'm finally getting back to this PR. This PR had broken two tests related to Let me know if there's anything I can do to facilitate reviews in order to get this PR merged in. |
c9b9409 to
35b0fc8
Compare
35b0fc8 to
48e28b8
Compare
|
I likely won't be able to review properly the changes since Oct. 8 before the holiday break. Sorry about that, this looks like great work. |





PR summary
This PR fixes several accuracy bugs with image resampling:
Below are illustrative plots and the generating script for linear resampling of a 2-pixel array. There are fundamental limitations of the accuracy due to the subpixel approach of the agg backend, but the motivation here is for the average behavior to be accurate (i.e., eliminate bias).
Before this PR
After this PR
Script
PR checklist