-
-
Notifications
You must be signed in to change notification settings - Fork 8k
Rewrite of image infrastructure #5718
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
Conversation
Away from my computer at the moment. Will pushing all of this stuff down
into AGG break things for non-agg backends such as macosx and Cairo?
On Dec 22, 2015 5:19 PM, "Thomas A Caswell" notifications@github.com
wrote:
Might be related to #3057
#3057 #1441
#1441 and #4584
#4584—
Reply to this email directly or view it on GitHub
#5718 (comment)
.
Away from my computer at the moment. Will pushing all of this stuff down into AGG break things for non-agg backends such as macosx and Cairo?
Our image interpolation is already done using the _image extension, which has always used Agg (but not backend_agg) for the heavy lifting, for all backends.
The change here (and I admit it's confusing and I wasn't very clear) is that the backend_agg used to do interpolation as well under certain circumstances (basically when interpolation='none' or there was a skew/rotation) rather than the _image extension. This was quite almost exactly the same code in _image and _backend_agg, except the latter supported rotation/skew. By moving rotation/skew into _image and then removing all image resampling from _backend_agg, the other raster backends (Cairo, OSX) now have the ability to rotate and skew images as well now and are on par with the features in the Agg backend in that regard for the first time.
Ah, that is much clearer! Sometimes I forget the distinction between the
use of agg and backend_agg.
On Dec 23, 2015 11:31 AM, "Michael Droettboom" notifications@github.com
wrote:
Away from my computer at the moment. Will pushing all of this stuff down
into AGG break things for non-agg backends such as macosx and Cairo?Our image interpolation is already done using the _image extension, which
has always used Agg (but not backend_agg) for the heavy lifting, for all
backends.The change here (and I admit it's confusing and I wasn't very clear) is
that the backend_agg used to do interpolation as well under certain
circumstances (basically when interpolation='none' of there was a
skew/rotation) rather than the _image extension. This was quite almost
exactly the same code in _image and _backend_agg, except the latter
supported rotation/skew. By moving rotation/skew into _image and then
removing all image resampling from _backend_agg, the other raster
backends (Cairo, OSX) now have the ability to rotate and skew images as
well now and are on par with the features in the Agg backend in that regard
for the first time.—
Reply to this email directly or view it on GitHub
#5718 (comment)
.
f677ac8 to
615c0dc
Compare
This is now passing on Travis, has some "What's New" entries, and is ready for final review.
@tacaswell: Thanks to the pointers to related issues. That was helpful for verifying some things outside of the existing test suite.
I'll give the C++ code a thorough read through tomorrow.
On Dec 30, 2015 5:40 PM, "Michael Droettboom" notifications@github.com
wrote:
This is now passing on Travis, has some "What's New" entries, and is ready
for final review.@tacaswell https://github.com/tacaswell: Thanks to the pointers to
related issues. That was helpful for verifying some things outside of the
existing test suite.—
Reply to this email directly or view it on GitHub
#5718 (comment)
.
master...matplotlib:6659dd704336b7575d8234b83a789531abd721ff
are links to the diffs excluding the svg files
examples/api/demo_affine_image.py picked up the execute bit for all, was that intentional?
in backend_svg it looks like the alpha value is only applied if the transform is not None, is that correct?
docstring of class ScalarMappable.to_rgba needs some updating.
L1251 is figure.py needs to be updated to match the same logic in Axes re if images can be compositied.
Would it be better to use dtype.kind (https://docs.scipy.org/doc/numpy-1.10.0/reference/generated/numpy.dtype.kind.html#numpy.dtype.kind) instead of issubclass(A.dtype.type, np.floating) ?
I have a some what pathological example where colors not from the color map end up in the final image.
import numpy as np import matplotlib.pyplot as plt X, Y = np.ogrid[-1000:1000, -1000:100000] R = np.sin((X**2 + Y**2) / 1000) fig, ax = plt.subplots() ax.imshow(R, cmap='viridis', interpolation='nearest') ax.set_aspect('auto') ax.set_xlim((28860.457192461989, 31279.898576044176)) ax.set_ylim((1044.2529963498005, 906.46842593439703)) ax.set_xscale('log') plt.show()
To be fair, this is a couple layers of stacked Moire patterns as out here the grid is way under sampling the underlying function to begin with and then it is being way down sampled.
I also found that the initial draw is faster under the new code, but panning is snappier under the old code.
examples/api/demo_affine_image.py picked up the execute bit for all, was that intentional?
Nope. Will fix. My emacs is set up to helpfully apply the executable bit if there's is #! in the file. One could argue a file with a hashbang without the executable bit is degenerate. Probably best to just remove the hangbang to be consistent with most of the other examples.
in backend_svg it looks like the alpha value is only applied if the transform is not None, is that correct?
Yes. If there is a transform passed, it gets exactly the original data, so the alpha needs to be applied by the renderer. If there is no transform passed, the alpha has already been applied by image.py. The reason for this is to properly support alpha-blending when images are pre-composited together. This did, however, illuminate one bug in the application of alpha in image.py and the fact that PDF isn't currently doing this correctly. PS simply doesn't support alpha blending so not much we can do there.
docstring of class ScalarMappable.to_rgba needs some updating.
done.
L1251 is figure.py needs to be updated to match the same logic in Axes re if images can be compositied.
Not sure I follow. Isn't that already the case? Compositing happens if there's
- more than one image
- the backend supports it
- none of the images have
interpolation == 'none'
It looks like it's the same in both places. (Could be refactored into a shared function, I suppose...)
Would it be better to use dtype.kind (https://docs.scipy.org/doc/numpy-1.10.0/reference/generated/numpy.dtype.kind.html#numpy.dtype.kind) instead of issubclass(A.dtype.type, np.floating) ?
I forgot about that... That's certainly more readable.
an update on my pathological case, if you zoom way in in the png it looks correct, the red is coming from something outside of mpl doing blending in RGB space.
Re L1251 Sorry, I was reading the first half of the diff and that was changed in the second half.
an update on my pathological case, if you zoom way in in the png it looks correct, the red is coming from something outside of mpl doing blending in RGB space.
What do you think that would be? -- "we control all the pixels" all the way down (at least for PNG). As far as I can tell, the "pinky" color is some sort of perceptual trick of the eyes when you have two very contrasting colors of only one pixel right next to each other (and personally, my display is 220dpi, so the pixels are quite small). If I zoom way in on the screenshot in the GIMP and use the eyedropper tool, however, I don't actually see any colors from outside the colormap. All this makes a very good stress test, though.
An optical illusions makes more sense. The display software (web browser / gimp) might be down-sampling to display, but Qt should not be.
If I take a screen shot of this page and zoom in on the (downsized) png gimp does show some red.
If I take a screen shot of this page and zoom in on the (downsized) png gimp does show some red.
I see. As you imply, I don't think there's anything we can do about that. As long as that doesn't happen from savefig or a screen shot directly taken from a GUI backend, we're fine.
edf0078 to
74723fa
Compare
You can download the image failures from the artifact tab of each not passing appveyor run. They include a nice html file which has the failing tests at the top.
-> https://ci.appveyor.com/project/mdboom/matplotlib/build/1.0.832/job/cnc0brawn35inqpw/artifacts
1f4e9eb to
fff1d66
Compare
I think this is working again, though Appveyor seems stopped up. Feel free to merge once passing.
Time for #6004...
This "is the test running too slow" test seems a bit finicky. AppVeyor is passing otherwise.
Rewrite of image infrastructure
Merged despite the too-slow failure.
Rewrite of image infrastructure Conflicts: lib/matplotlib/tests/test_axes.py - do not back-port appveyor spceific limits lib/matplotlib/tests/test_image.py - do not backport the jpeg_alpha test setupext.py - do not include windows/appveyor related changes
backported to v2.x as 6595139
\o/
This began as an attempt to fix #5490, but it turned out to be impossible to fix that without addressing a number of other issues.
There's a number of things this does. Let's start with fixing #5490.
The order of operations has changed from:
to
I had experimented with doing the normalization after the resize, but Agg is designed for data in range (0, 1). It might actually be more mathematically/perceptually correct to do the normalization before the resizing (which does linear interpolation modulo some filter) so that if a non-linear norm is being applied, the interpolation takes that into account.
BEFORE:
test
AFTER:
good
You can see the images no longer have the "red" that doesn't belong to the color map.
In addition:
image.pywas almost completely rewritten. There was a lot of copy-and-pasted code in there that has now been factored out by having all the image classes inherit from the same base class.svg.image_noscalercParam is not longer supported. One can get the same behavior by settinginterpolation=Noneon images orimage.interpolation: NonercParam globally._image_skew_coordinatehas been removed. One can set the transform on the image object or its parent Axes to get the same behavior, and this will be understood by other artists as well so I think is much preferred.image.resampletoTrue. The difference there isresample: Falseonly applies interpolation when upsampling.resample: Trueapplies it in both directions. And it has really odd behavior when one dimension is upsampled and the other is downsampled. I believe the history of this is thatresample: Falseis the old behavior before our Agg backend had full-on resampling in it, and the flag was maintained to allow for backward-compatible performance. I think it's high-time to use the more modern approach by default, especially wrt the upcoming 2.0 release._imageextension and the agg backend, apparently so that the Agg backend could apply an affine transform to the image like some of our vector backends. To reduce the number of complex code paths, the Agg backend has been reverted to being a "dumb" backend that can't resize an image and all resampling happens before it hits the backend. This doesn't break theinterpolation='none'feature where an exact copy of the original data is stored in a vector file.draw_imagebackend API has been simplified. It now simply takes an optional affine transform if the backend can handle transforming the image itself (this is true of all vector backends, and none of the others).