Support cartopy 0.20 and proj 8.1.0 #4331

bjlittle · 2021-09-21T07:49:46Z

🚀 Pull Request

Description

This PR includes the changes to support cartopy 0.20 and proj 8.0.1.

Also see the companion PR SciTools/test-iris-imagehash#53 for the imagehash changes, which will require to be merged for this PR to pass.

Ref: SciTools/cartopy#1252

Consult Iris pull request check list

docs/gallery_code/general/plot_projections_and_annotations.py

bjlittle · 2021-09-21T08:18:38Z

lib/iris/tests/integration/experimental/test_regrid_ProjectedUnstructured.py

-                [318.935163, 318.935163, 318.935163],
+                [318.92881733, 318.92881733, 318.92881733],
+                [318.92881733, 318.92881733, 318.92881733],
+                [318.92881733, 318.92881733, 318.92881733],


I can't identify the root cause for the change in numbers here... hence why I've introduced a minimum pin for cartopy in iris (after all, this is a major release update of proj)

We either accept this change "as is"... or dig deeper.

What is the provenance of the original numbers in the test? Assuming these are degrees longitude the difference is 0.00635deg ~ 700m on the equator.

Do we at least understand why the original test array was 2 rows of 318.936829 and 1 row of 318.935163, while the replacement is 3 identical rows of 318.92881733?

What is the provenance of the original numbers in the test? Assuming these are degrees longitude the difference is 0.00635deg ~ 700m on the equator.

I don't think there is any significance to the values being tested in the array, other than a random snapshot. @pp-mo Any ideas on the history of this one?

I think the philosophy of the test is rather weak, in that it's attempting to give "some level" of confidence that nothing has changed... but when something does change (like now) there is no context to determine whether the change is significant and/or valid.

I'm more inclined to trust a plot rather than raw numbers for this type of integration test 🤔

I think the philosophy of the test is rather weak, in that it's attempting to give "some level" of confidence that nothing has changed... but when something does change (like now) there is no context to determine whether the change is significant and/or valid.

👍 😁

@pp-mo Any ideas on the history of this one?

I don't think there is anything special here, just a desire to check a small section of individual points, not just statistics over the whole array.

An actual change equivalent to 100's of metres does seem rather large though. Maybe the default ellipsoid for this changed ?
I would have suggested that we should apply a more sensible tolerance to AssertArrayEqual.
Its default is 6DP, so that ~2cm (!!)
But to tolerate this we would need to have decimal=1 .
0.1 degrees ~1700m does seem rather large.

Then again... the plots are rather different too...

The left plot is cartopy 0.19.0.post1, and the right plot is cartopy 0.20.0

The six subplots within each plot are the six levels [0..5] in the timeseries, where level[0] is the top-left and level[5] is bottom-right.

Apologies, forgot to add a colorbar, oops.

There is some similar structure there, but the differences are (to me) significant.

At this point, I can't confirm what's right. Any clues @pp-mo ?

actual values shouldn't change. But the location of the values can.

how did this manage to match on the array stats, I wonder ?

I've rebased, as this PR went stale...

I'm still in favour of creating an issue to cover this, if that seems like a pragmatic compromise. Then we can dedicate some time to digging a bit further to understand the root cause of the issue (if possible), but bank this PR and move on.

I'm still in favour of creating an issue to cover this, if that seems like a pragmatic compromise. Then we can dedicate some time to digging a bit further to understand the root cause of the issue (if possible), but bank this PR and move on.

If we're honest, that's effectively saying we agree this change isn't a problem, and maybe we'll get round to working out what caused it at some point in the future. I'm fine with that if everyone else is.

However: if this is agreed to be a real concern then I'm very uncomfortable merging into main. If it's a real concern and we don't have time now then we should put a maximum pin on Cartopy, no matter how horrible that prospect is.

Does the fact that this is in experimental influence how much we should worry about this?

@rcomer Nice point, it's experimental... I'm not too precious about it.

Also, the fact that this issue is isolated in one test and not an issue peppered throughout Iris is somewhat comforting.

lib/iris/tests/integration/plot/test_vector_plots.py

lib/iris/tests/results/unit/analysis/cartography/project/TestAll/cube.cml

lib/iris/tests/unit/analysis/cartography/test_project.py

requirements/ci/py38.yml

pp-mo · 2021-09-21T09:35:07Z

@jamesp I'm inclined to remove myself from review/assign + let you take it on instead.
Are you good to do that ?

jamesp · 2021-09-21T10:11:34Z

@pp-mo fine with me!

@bjlittle I see no reason not to merge this, seems reasonable changes in response to underlying dependency change. Why are the tests still failing?

bjlittle · 2021-09-21T10:17:58Z

@jamesp Someone needs to review and merge SciTools/test-iris-imagehash#53 😄

pp-mo · 2021-09-21T11:19:26Z

@jamesp Why are the tests still failing?
@bjlittle Someone needs to review and merge SciTools/test-iris-imagehash#53

I've merged that + re-spun the tests.
No such luck with the benchmark failure though ?

jamesp · 2021-09-21T12:16:19Z

Benchmark failure is very strange - it seems to be installing proj 7 instead of proj 8, but the lockfile at HEAD looks like it has proj 8

bjlittle · 2021-09-22T10:08:20Z

benchmarks/conda_lock_plugin.py

+        installed_hash = self._get_installed_commit_hash()
+        if installed_hash:
+            log.info(f"Clearing conda environment for {self.name}")
+            self._cache._remove_cache_dir(installed_hash)


Thanks to @jamesp for this magic fix 🎉

pp-mo · 2021-09-30T13:10:04Z

Let's bank the "new" numbers, and create an issue to investigate this.
It will presumably fail if run with older Proj, but I think the new cartopy>=0.20 pin should make that "not a problem".

Issue to cover : #4346

* Updated environment lockfiles * Support cartopy 0.20 and proj 8.1.0 (#4331) Co-authored-by: Lockfile bot <noreply@github.com> Co-authored-by: Bill Little <bill.james.little@gmail.com>

bjlittle added Type: Infrastructure New: Pull Request labels Sep 21, 2021

bjlittle commented Sep 21, 2021

View reviewed changes

docs/gallery_code/general/plot_projections_and_annotations.py Show resolved Hide resolved

bjlittle commented Sep 21, 2021

View reviewed changes

lib/iris/tests/integration/plot/test_vector_plots.py Show resolved Hide resolved

bjlittle commented Sep 21, 2021

View reviewed changes

lib/iris/tests/results/unit/analysis/cartography/project/TestAll/cube.cml Show resolved Hide resolved

bjlittle commented Sep 21, 2021

View reviewed changes

lib/iris/tests/unit/analysis/cartography/test_project.py Show resolved Hide resolved

pp-mo self-assigned this Sep 21, 2021

pp-mo self-requested a review September 21, 2021 08:25

bjlittle changed the base branch from main to auto-update-lockfiles September 21, 2021 08:39

jamesp reviewed Sep 21, 2021

View reviewed changes

requirements/ci/py38.yml Show resolved Hide resolved

bjlittle changed the title ~~Update~~ Support cartopy 0.20 and proj 8.0.1 Sep 21, 2021

bjlittle commented Sep 22, 2021

View reviewed changes

github-actions bot force-pushed the auto-update-lockfiles branch from b441d02 to de38d1c Compare September 25, 2021 00:55

bjlittle added 2 commits September 28, 2021 11:54

support cartopy 0.20 and proj 8.0.1

0700aa3

fix gha benchmarking

1f96e4b

bjlittle force-pushed the update branch from 39a99d5 to 1f96e4b Compare September 28, 2021 10:55

bjlittle changed the title ~~Support cartopy 0.20 and proj 8.0.1~~ Support cartopy 0.20 and proj 8.1.0 Sep 28, 2021

pp-mo mentioned this pull request Sep 30, 2021

Review regridding result change from Cartopy v0.19 to v0.20 #4346

Closed

pp-mo merged commit 8846e5d into SciTools:auto-update-lockfiles Sep 30, 2021

bjlittle mentioned this pull request Sep 30, 2021

missing whatnew entry #4348

Merged

rcomer mentioned this pull request Oct 1, 2021

Remove Cartopy 0.19 workaround #4349

Merged

bjlittle deleted the update branch January 11, 2022 11:11

rcomer mentioned this pull request Aug 1, 2022

Sphinx reset modules #4885

Merged

1 task

rcomer mentioned this pull request Aug 11, 2022

Lockfile update and skip questionable test #4901

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support cartopy 0.20 and proj 8.1.0 #4331

Support cartopy 0.20 and proj 8.1.0 #4331

bjlittle commented Sep 21, 2021 •

edited

Loading

bjlittle Sep 21, 2021 •

edited

Loading

jamesp Sep 21, 2021

bjlittle Sep 21, 2021

pp-mo Sep 21, 2021

bjlittle Sep 21, 2021

pp-mo Sep 28, 2021

bjlittle Sep 28, 2021

trexfeathers Sep 28, 2021

rcomer Sep 28, 2021

bjlittle Sep 28, 2021 •

edited

Loading

pp-mo commented Sep 21, 2021

jamesp commented Sep 21, 2021

bjlittle commented Sep 21, 2021 •

edited

Loading

pp-mo commented Sep 21, 2021

jamesp commented Sep 21, 2021

bjlittle Sep 22, 2021

pp-mo commented Sep 30, 2021 •

edited

Loading

Support cartopy 0.20 and proj 8.1.0 #4331

Support cartopy 0.20 and proj 8.1.0 #4331

Conversation

bjlittle commented Sep 21, 2021 • edited Loading

🚀 Pull Request

Description

bjlittle Sep 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjlittle Sep 28, 2021 • edited Loading

Choose a reason for hiding this comment

pp-mo commented Sep 21, 2021

jamesp commented Sep 21, 2021

bjlittle commented Sep 21, 2021 • edited Loading

pp-mo commented Sep 21, 2021

jamesp commented Sep 21, 2021

Choose a reason for hiding this comment

pp-mo commented Sep 30, 2021 • edited Loading

bjlittle commented Sep 21, 2021 •

edited

Loading

bjlittle Sep 21, 2021 •

edited

Loading

bjlittle Sep 28, 2021 •

edited

Loading

bjlittle commented Sep 21, 2021 •

edited

Loading

pp-mo commented Sep 30, 2021 •

edited

Loading