Conversation
|
I've pushed a followup that changes tests that were already affected to use the mpl20 style. This is more modern and also minimizes the RMS tolerance we need in most cases. Probably these two commits can be squashed, but I kept them separate if you wish to review them by themselves. |
| @pytest.mark.mpl_image_compare(filename='xticks_no_transform.png', style='mpl20', | ||
| tolerance=5.20 if not _MPL_311 else 0.5) | ||
| def test_set_xticks_no_transform(): | ||
| plt.rc('figure.subplot', left=0.125, right=0.9, bottom=0.1, top=0.9) |
There was a problem hiding this comment.
Note in test_images, I went with changing the axes() call to add_axes with this rect:
https://github.com/SciTools/cartopy/pull/2685/changes#diff-ef0d6ab6699444c9c1bdbb5f5003c73a10781b3d70cec64f3009344a4d3a647dR129
I don't know if one way looks better than the other.
| tolerance=5.42 if not _MPL_311 else 0.5) | ||
| def test_set_xyticks(): | ||
| plt.rc('figure.subplot', left=0.125, right=0.9, bottom=0.1, top=0.9) | ||
| plt.rc('axes.formatter', limits=(-7, 7)) |
There was a problem hiding this comment.
This prevents the y-axis from changing to a smallish number with a 1e6 multiplier.
rcomer
left a comment
There was a problem hiding this comment.
Thanks for doing this @QuLogic.
I think we can get rid of the largest of these tolerances by using your text_placeholders fixture (see my comments on the images). I'm not sure about the others: I never got the hang of what counts as too high for a tolerance.
721bba0 to
1121b71
Compare
|
Ah, it looks like the placeholders didn't handle rotation, so the boxes stick together a little differently on macOS. I'll try and see if I can get the placeholders to rotate as well. |
|
Will we always need to vendor the fixture or will we be able to import it from Matplotlib when we require a high enough version? |
|
After a bit of CI debugging, I've found that on Ubuntu (x86_64) vs macOS (arm64), there are some small differences in the map boundary and subsequent gridline calculations. The map boundary difference is only somewhere below the 12th decimal place though, so that would seem not very relevant. What does differ more significantly is the calculated On Ubuntu (x86_64), the 20°S, 0, and 40°N lines are split into 4 segments, whereas 20°N, 60°N, and 80°N are only 2 segments. On the hand, with macOS (arm64), the 60°N and 80°N lines are also split into 4. This occurs roughly at the ends +/-180° longitude, which on the LCC projection for the test is at about 60° from the top-left edge. You can see a comparison here: It may be a bit difficult to see, but there are a few extra colours along that split for the two innermost latitudes. For cartopy/lib/cartopy/mpl/gridliner.py Lines 813 to 839 in c1b512a Now, it turns out that when we get 4 parts, each smaller part is just two elements long and an end is coincident with an end of a longer part. I'm uncertain why we end up with these extra parts (as at least the transformed latitude line is the same on both platforms, and the split is nowhere near the map boundary), but if we merge them together, then we can get consistent potential-label-positions on all platforms and they end up all being visible in the same way. As you can see in the above code, we already merge the lines if there are more than 4 parts. I do not have a strong idea of whether we should always have one |
I didn't end up applying any rotation changes, so yes, we can import it when we require a high enough version. |
Sigh, and now macOS produces a different result for that one... Let me see what the calculations are like. |
|
So I guess I see this Robinson case is the reason why we have that merging code for |
|
OK, I've had to isolate the changes in the last commit, so that if after line merging we still have to combine all the sub-geometries manually, then do that on the pre-line-merged intersection. This avoids the platform-specific re-ordering that Shapely/GEOS seems to be doing. If I can make a reproducer, I will report this to whichever upstream, but in the meantime, this will allow us to get tests working. |
|
Thanks for the detailed explanations @QuLogic. These changes all makes sense to me. I think we just need to decide what to do with the |
I also compressed the results with `oxipng -o max`, which saved: ``` Files processed: 57/57 Input size: 4.45 MiB (4663311 bytes) Output size: 2.02 MiB (2121057 bytes) Total saved: 2.42 MiB (54.52%) ```
Also, use the RdBu colour map in `test_contour_label` instead of the default (previously jet, now viridis), which fits the data better.
This seems to be different on macOS.
Sometimes, Shapely will produce a `MultiLineString` with multiple parts that coincide (most notably on macOS for `LambertConformal`), causing potential labels to be placed in slightly different places, and then different ones to be visible due to overlap. By merging the intersection of the lines and map boundary, we can ensure that potential labels are placed in consistent spots, and there should not be any platform-specific changes to lable visibility. Unfortunately, there does still appear to be a bit of inconsistency between platforms as far as normalization goes, so we need to fall back to the original intersection result if we are merging a large number of sub-geometries.
rcomer
left a comment
There was a problem hiding this comment.
Please handle the merge however you would like.

Rationale
As we all know, Matplotlib changed text rendering, as well as some image resampling code, so tests are all failing when running against the latest version.
I went for replacing the images and adding a tolerance for older versions. This does result in quite high tolerances in some cases, so it is possible we may wish to ship two images for some time instead.
Implications
Compatibility with latest release of Matplotlib.
I also compressed the results with
oxipng -o max, which saved:Fixes #2666