Unpin Matplotlib and proj

[stephenworsley]

Addresses #3759 and #3154.

It is expected that many tests may have to change to accommodate this. It should be demonstrated in each case that such a change is appropriate.

[stephenworsley]

The test test_extent/test_cube (iris.tests.unit.analysis.cartography.test_project.TestAll) is failing due to a change in the numerical result. This may be due to improvements in the accuracy of Proj in the Robinson projection made, for example, here OSGeo/PROJ#1292.

[trexfeathers]

Installing Cartopy now installs GDAL (see changes to the Cartopy recipe). Is it therefore appropriate to remove GDAL from iris/requirements/extensions.txt, or is it good to keep this as 'belt and braces'?

[pp-mo]

Is it therefore appropriate to remove GDAL from iris/requirements/extensions.txt

Looking at what was done here, I now think it was a mistake to put it in that list in the first place (and I did it!) : The comment suggests that the 'extensions' should be things that depend on Iris itself (hence iris-grib). But equally, it's not clear when this information would ever used : not by Travis, not by pip, so presumably it's just a hint to users.

In short, I would remove it + not worry.

[stephenworsley]

I have a hunch that this PR is the one which caused the GDAL related tests to fail OSGeo/gdal#1185.
Unfortunately, it's not a small PR so it's a bit tricky to unpick what's going on here, but relevant to our tests are these changes https://github.com/OSGeo/gdal/pull/1185/files#diff-3610c5e11cf9b7bc8e8b33baa7f4ca70. Specifically the change of certain strings from "unnamed" to unknown.

[trexfeathers]

In light of SciTools/cartopy#1105, I've changed all tests that use ax.coastlines() to explicitly state the target resolution. This will not yet fix the relevant graphics tests as there are still some minor border differences to accept - will review those with @stephenworsley tomorrow.

SciTools/cartopy#1105 has also caused a failure in test_plot_custom_aggregation (see the relevant Travis build), but in this case I propose we should accept the new result as the correct one since it will henceforth demonstrate Cartopy's AdaptiveScalar functionality, which makes Iris simpler to use. In this situation would it be appropriate to also remove the old 'out-dated' image hash(es) as no longer acceptable?

[trexfeathers]

Given the discussion on #2952, we should probably have a wider discussion about the implications of Cartopy now bringing in GDAL as a core dependency.

• Size: I compared the size of a conda environment for Iris master (a4541ce) and this PR (da2977e)
  I think this is a serious cause for concern, particularly at a time when we are discussing reducing Iris' install size:
  • python requirements/gen_conda_requirements.py > [my file name]
  • conda create -n [my env name] --file [my file name]
  • du -sh [path to my conda environment]
  • master: 604M
    Unpin Matplotlib and proj #3762: 1014M
  • Haven't yet confirmed if GDAL is responsible for this, although based on previous discussions it seems likely
• Constraints/pins: the GDAL recipe is now significantly less pinned than it when at the time @pp-mo raised Stop testing with gdal. #2952
• Possible other concerns - I need to have a thorough read of Stop testing with gdal. #2952 to see how the discussion relates to the current scenario

[trexfeathers]

Graphics test failures

Accepted image hash changes (SciTools/test-iris-imagehash#35):

• Some behaviours have changed permanently - either through pinning cartopy>=0.18 or Iris functionality changes - so I have elected to set new image hashes as the only acceptable targets for:
  • efda068 Shows off the auto-scaling coastlines (Utilise AdaptiveScalar in default NaturalEarthFeatures cartopy#1105) in the custom aggregation gallery example
  • 4050012 Reflects the new grid-line spacing (SciTools/cartopy@2f5e568)
  • 27ea2f2 + 27688ca Now expecting a different axis label for raw plotting of time points on an axis
    (See also 27b0602, 45781b5, 484a611 for the rest of the work on this)
• 41b3b2a I have accepted some minor changes in color map ranges (barely perceptible) based on the fact this has happened with previous Matplotlib shifts (e.g. Unpin mpl test-iris-imagehash#26 - compare before and after).
  (See also ef3c9db for an extra change)

[trexfeathers]

"What's New" Entries

Changes here will need a "What's New", but it will be simpler to add after merging - see #3807

[trexfeathers]

All the graphics tests are now passing. @stephenworsley I'll leave you to continue investigating the other test failures. Let me know if you need a hand 🙂

[stephenworsley]

While I still haven't got to the root cause of why the behaviour has changed, I've looked through the documentation for the key 42113 (https://gdal.org/drivers/raster/gtiff.html#nodata-value) and there is nothing to indicate that storing a fill value for data without masked points is in any way improper.

[stephenworsley]

It looks like there is still something odd going on with test_cube. I got it working in my own environment so that the numbers were correct, but it looks like there's still something going on with the Travis environment causing there to be inf values. It seems that this was introduced between Proj 7.0.0 and 7.1.0.

[stephenworsley]

The missing values mentioned above may be related to this PR OSGeo/PROJ#2151.

[trexfeathers]

Given the discussion on #2952, we should probably have a wider discussion about the implications of Cartopy now bringing in GDAL as a core dependency.

Several devs have further discussed offline. GDAL offers a number of benefits, and currently would only be responsible for a fraction of Iris' total install 'bloat', so we expect will be fine to include. We are already familiar with use cases that make regular use of GDAL/Fiona with Iris.

If we make progress with reducing Iris' install size ('Iris Slimline'), GDAL's inclusion may be of greater concern. If so, there may be a case to alter Cartopy's inclusion of GDAL in its Conda recipe: the changes to that recipe for 0.18 (conda-forge/cartopy-feedstock#88) no longer reflect the fact that GDAL is actually still an optional dependency - it can still be seen to be optional in cartopy/INSTALL and the absence of GDAL in the "Minimum dependencies" matrix entry of cartopy/.travis.yml (instead only brought in with Fiona in the "Latest everything" matrix entry).

[trexfeathers]

... are we there?

[stephenworsley]

@trexfeathers close I think, just needs some reviewing now.

[trexfeathers]

OK, please can you confirm that you are happy with the changes I have made? We discussed almost all of this last week.

I will review the changes you have made and once we are both happy I will merge.

[trexfeathers]

This review is a text recording of a voice conversation during pair programming, where @stephenworsley explained the changes that they have made in the PR.

[stephenworsley]

The changes due to graphical tests look good to me.

[trexfeathers]

@stephenworsley and I have finished pair programming and pair reviewing this PR. @pp-mo we'll give you some time to comment/review if you wish, but we consider this good to go 💯

[pp-mo]

@pp-mo we'll give you some time

Ok I've not considered everything here at all, only details, but I'll take a little look + see if anything sticks out...

[pp-mo]

Just to note from previous comment ...

In this situation would it be appropriate to also remove the old 'out-dated' image hash(es) as no longer acceptable?

Likewise, from other comment

Some behaviours have changed permanently ... I have elected to set new image hashes as the only acceptable targets

I think we don't generally bother with this too much. The chances of a bug producing a plot that matches an older result, which is now a wrong answer, are supposed to be pretty small. But it doesn't seem 'wrong' to do it.
However, we definitely don't want to remove the images from test-iris-imagehash : That would prevent running the tests on an older checkout, and we would generally prefer to keep that working, if possible (though dependency changes may still mess it up).

[pp-mo]

A lot of very careful work here ! 💐
Gets my vote. @trexfeathers , when ready : hit that button 👍
( you deserve it ! )

[trexfeathers]

However, we definitely don't want to remove the images from test-iris-imagehash : That would prevent running the tests on an older checkout, and we would generally prefer to keep that working, if possible (though dependency changes may still mess it up).

I agree and accordingly I haven't done anything with SciTools/test-iris-imagehash.

[xylar]

I just noticed this discussion. I'm sorry me adding gdal as a dependency of cartopy caused you trouble. You are right in #3762 (comment) that it should have been kept optional. I'm removing it as a required dependency in the next build: conda-forge/cartopy-feedstock#94 but wanted to check in to make sure this isn't going to cause additional trouble.

[trexfeathers]

but wanted to check in to make sure this isn't going to cause additional trouble.

@xylar luckily we're currently doing some work that should flush out any possible recipe issues, so we should be able to let you know in the next couple of days. Don't think we're expecting any problems though 🙂

[xylar]

Okay, I'm going to go ahead an merge conda-forge/cartopy-feedstock#94 unless you would prefer I hold off.