🌐 AI搜索 & 代理 主页
Skip to content

Conversation

@ayshih
Copy link
Contributor

@ayshih ayshih commented Jun 18, 2025

PR summary

This PR fixes several accuracy bugs with image resampling:

  • The filtering weight at one extreme is not being calculated correctly. In the case of linear interpolation, this makes the one weight that should be zero be instead nonzero. This then results in a pixels of constant values manifesting a small blip after linear interpolation if the center of an input pixel exactly coincides with the center of a output pixel (within the agg backend, its pixels may not be exactly aligned with the true pixels).
  • The filtering weights are not aligned correctly in their array as expected by subsequent code. For a weight array of N values, the "pivot" needs to be at N / 2 - 1, not at N / 2. This results in a bias where the linearly interpolated values are calculated slightly to the right of where they should be calculated.
  • Specific to nonaffine transforms, the values in the lookup table used to store the calculation are being truncated when they should be rounded. This results in a bias (smaller than the above) to the right.
  • [subsequently added, with updated images therein] Specific to affine transforms, the end point for interpolation is not specified correctly, which can slightly degrade the accuracy of the sampling locations.

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

before

After this PR

after

Script

import matplotlib.pyplot as plt
import numpy as np
from matplotlib.transforms import Affine2D, Transform
from matplotlib.image import resample, BILINEAR

in_data = np.array([[0.1, 0.9]])
in_shape = in_data.shape
in_edges = np.arange(in_shape[1] + 1)

out_shape = (1, 20)
out_edges = np.arange(out_shape[1] + 1)

ideal_data = np.array([[0.1, 0.1, 0.1, 0.1, 0.1, 0.14, 0.22, 0.3, 0.38, 0.46,
                        0.54, 0.62, 0.7, 0.78, 0.86, 0.9, 0.9, 0.9, 0.9, 0.9]])

# Create a simple affine transform for scaling the input array
affine = 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 transform
class NonAffineIdentityTransform(Transform):
    input_dims = 2
    output_dims = 2

    def inverted(self):
        return self
nonaffine = NonAffineIdentityTransform() + affine

affine_data = np.empty(out_shape)
nonaffine_data = np.empty(out_shape)
resample(in_data, affine_data, affine, interpolation=BILINEAR)
resample(in_data, nonaffine_data, nonaffine, interpolation=BILINEAR)

fig, 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')

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('Interpolated data')

axs[2].stairs(affine_data[0, :] - ideal_data[0, :], out_edges, label='affine')
axs[2].stairs(nonaffine_data[0, :] - ideal_data[0, :], out_edges, ls='dashed', label='nonaffine')
axs[2].grid(ls='dotted')
axs[2].set_xticks(out_edges)
axs[2].set_ylim(-0.006, 0.006)
axs[2].legend()
axs[2].set_title('Interpolated data minus ideal result')

plt.show()

PR checklist

if(i <= pivot) m_weight_array[pivot - i] = value;
}
unsigned end = (diameter() << image_subpixel_shift) - 1;
m_weight_array[0] = m_weight_array[end];
Copy link
Contributor Author

@ayshih ayshih Jun 18, 2025

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.

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, done

Copy link
Contributor Author

@ayshih ayshih Jun 18, 2025

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.

Copy link
Member

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.

Copy link
Contributor Author

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!

@ayshih
Copy link
Contributor Author

ayshih commented Jun 18, 2025

  • The filtering weight at one extreme is not being calculated correctly. In the case of linear interpolation, this makes the one weight that should be zero be instead nonzero. This then results in a pixels of constant values manifesting a small blip after linear interpolation if the center of an input pixel exactly coincides with the center of a output pixel (within the agg backend, its pixels may not be exactly aligned with the true pixels).

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]]

@jklymak
Copy link
Member

jklymak commented Jun 18, 2025

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?

@ayshih ayshih force-pushed the more_resample_bugs branch from 7eb1cea to 5cbb7bb Compare June 18, 2025 15:31
@ayshih
Copy link
Contributor Author

ayshih commented Jun 18, 2025

This killed the log-scale images?

Indeed, I need to track down why.

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?

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.

@jklymak
Copy link
Member

jklymak commented Jun 18, 2025

Fair enough - however, it would be good if there were some tests devised where it showed a visual effect of these inaccuracies.

@ayshih ayshih force-pushed the more_resample_bugs branch 3 times, most recently from e198af6 to 717a221 Compare June 18, 2025 20:39
}
}

#ifndef MPL_FIX_IMAGE_FILTER_LUT_BUGS
Copy link
Contributor Author

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.

@ayshih ayshih force-pushed the more_resample_bugs branch 7 times, most recently from 43b912c to 90bd95f Compare June 19, 2025 19:20
@ayshih ayshih marked this pull request as ready for review June 19, 2025 20:47
@ayshih
Copy link
Contributor Author

ayshih commented Jun 19, 2025

I fixed the issue with log_scale_image, so this PR is now ready for review. The final count is 65 baseline images that need to be updated. Maybe a third of those images have (subtle) changes in the rendering of content, e.g.:

Before this PR:
imshow_masked_interpolation-expected

After this PR:
imshow_masked_interpolation

Difference:
imshow_masked_interpolation-failed-diff

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.

@QuLogic
Copy link
Member

QuLogic commented Jun 26, 2025

Are you sure you have the right set of images? lib/matplotlib/tests/baseline_images/test_axes/formatter_ticker_004.png for example doesn't have any images to be resampled.

@QuLogic
Copy link
Member

QuLogic commented Jun 26, 2025

OK, I see that RendererAgg::draw_text_image uses image_filter_spline36 when rotating text, so it only affects tests with rotated text. So for any tests that are purely rotated text, we should just increase the tolerance and make a note of them, then revert the tolerance changes in #30161 when regenerating the images. Or re-target this PR to the text-overhaul branch without test image changes (but then they'd all be regenerated much later.)

@ayshih ayshih force-pushed the more_resample_bugs branch from 90bd95f to 9fdca21 Compare July 10, 2025 13:51
@QuLogic
Copy link
Member

QuLogic commented Sep 30, 2025

Since you're deep into this code right now, I wonder if you have any ideas about #14143?

@ayshih
Copy link
Contributor Author

ayshih commented Sep 30, 2025

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.

@ayshih ayshih force-pushed the more_resample_bugs branch from 5488307 to 19ca801 Compare October 8, 2025 14:48
@ayshih
Copy link
Contributor Author

ayshih commented Oct 8, 2025

(by using the algorithm I mentioned above, making sure to always update both sides, and then nudging the center filter value up by one or down by one if needed)

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 w for a filter radius of 2 has 2 * 2 * 256 = 1024 elements, and the pivot is at index 511. For symmetry, w[510] should equal w[512]. However, those two weights are part of two separate normalization constraints: w[254] + w[510] + w[766] + w[1022] = 16384 and w[0] + w[256] + w[512] + w[768] = 16384. So, requiring symmetry after nudging w[510] up to fix one normalization would mean also nudging up w[512], which could potentially mean having to nudge down a third weight (w[0], w[256], or w[768]). Due to symmetry, that would mean nudging down a fourth weight, which then couples to a third normalization constraint, and so on.

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.

@ayshih
Copy link
Contributor Author

ayshih commented Oct 8, 2025

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 nearest-neighbor interpolation, one wants edge-aligned subpixels. That way, the edges as defined by subpixels looks exactly the same as the edges defined by the original pixels.
  • For all other types of interpolation, one typically wants center-aligned subpixels. That way, there are interpolation points at the center of original pixels and at the edges between pixels.

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:
Figure_2
Figure_1

@ayshih
Copy link
Contributor Author

ayshih commented Dec 5, 2025

I'm finally getting back to this PR. This PR had broken two tests related to SegmentedBivarColormap because SegmentedBivarColormap calls the Agg-based resampler to generate its lookup table (LUT), and of course the resampler now gives slightly different results. In my opinion, SegmentedBivarColormap should not be calling the Agg-based resampler because the Agg's internal use of subpixels means that LUT values are not quite correct (again, in my opinion). That said, I think that any change to the LUT calculation methodology is out of the scope of this PR. Instead, I have simply added a subpixel shift to the LUT calculation so that the SegmentedBivarColormap LUT is the same before and after this PR.

Let me know if there's anything I can do to facilitate reviews in order to get this PR merged in.

@ayshih ayshih force-pushed the more_resample_bugs branch 2 times, most recently from c9b9409 to 35b0fc8 Compare December 5, 2025 15:38
@anntzer
Copy link
Contributor

anntzer commented Dec 9, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants