-
Notifications
You must be signed in to change notification settings - Fork 389
Move GeoAxes.background_patch to GeoAxes.patch. #1422
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
|
In general this looks good. Everything seems cleaner. Waiting to see what the image changes look like.. |
|
Currently, it's mostly the same as the referenced Matplotlib issue, plus matplotlib/matplotlib#15946. |
|
Looks great to me. I just pulled your code and this does appear to fix the tight layout issues. Specifically, #1207 looks good to me locally with your changes. With constrained_layout it appears much better, but there is still a slight shift between the two figures. |
|
For reference, I've committed the changed images and the diffs here. To understand most of the changes, you have to know that Matplotlib draws spines at 0.8 linewidth, so there's usually one middle pixel full black, and two surrounding pixels of grey. Correctly clipped artists should end at the inner grey or the black pixel, and the inner grey one might be shaded a bit by the artist's colour. If not clipped correctly, then they might mix into the outer grey pixels, or only mix into the inner grey one via antialiasing (and thus a bit darker than they should be.) My analysis of the changes is:
|
|
Are you going to close this draft PR and merge all the changes in that other link you sent?
Do you want to update this image if it is a known bug, or just xfail it in pytest for now and then update the image once that is fixed in MPL upstream? |
|
@QuLogic, are you still planning on doing more work on this? I think it is an excellent improvement and well worth getting in for the next release. |
|
I still want to do this for 0.18, but I'm looking to fix the two bugs in Matplotlib first. I had half-fixed one, and the other should be possible, though it needs further testing. |
34f8efa to
07c2e12
Compare
1793888 to
f6bc67e
Compare
|
I've now run through the whole combination of dependencies, and updated the tests to account for the change here. I updated the images where stuff was improved and where differences were small-ish, I just increased the tolerance (note our default is 0.5 RMS.) Replacement images were generated with the latest Matplotlib, and tolerances added for older versions if necessary. Some much older versions of Matplotlib are completely broken for some tests, but rather than fix that, I just decided to mark the tests as Unfortunately, I was not able to fix some of the above bugs in Matplotlib for 3.2.0, so I just upped the tolerances for the tests that were affected here. |
|
Oh, and one other is |
greglucas
left a comment
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.
I think this is a nice abstraction and all of the new images look good. 👍 from me, just a minor comment and question. Thanks for the great effort seeing this through!
| square in axes coordinates. | ||
| """ | ||
| if use_as_clip_path is not None: |
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.
Do you know why this was there in the first place?
If I'm understanding this correctly, everything will now be clipped at the defined boundary no matter what. Whereas before, there was an option to not use the map boundary as a clip path, but rather just use the default axes.
What you're doing makes intuitive sense to me, I'm just wondering if there would be a reason to keep this?
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.
I prefer that people use Matplotlib's builtin support for clipping, by setting clip_on=False on the artists they don't want to be clipped.
The regrid_image.png test image is affected by this, but honestly it looks better this way (note, not updated yet due to reasons above.)
f6bc67e to
43421a7
Compare
In earlier versions, the facecolor of GeoAxes objects is set via GeoAxes.background_patch.set_facecolor() see: SciTools/cartopy#880 This is changed in 0.18: SciTools/cartopy#1422 To make the grey areas in helpers/map_type_handling.py work, cartopy>=0.18 is required.
Rationale
This is a followup to #1213, converting the Cartopy-specific
background_patchinto the usualAxes.patchfrom Matplotlib.Implications
Unfortunately, this breaks several tests. Most of these are due to clipping bugs upstream in Matplotlib (at least matplotlib/matplotlib#15952). I'm hoping to get these fixed, regenerate any test images with the new version if necessary, then just increase tolerance for the old images (most of them are small changes anyway.)
Since we aren't using the default patch, this should fix tight layout too, but I haven't yet tested it.
Fixes #1207.