Skip to content
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

[iris.ci] environment lockfiles auto-update #4967

Closed
wants to merge 1 commit into from

Conversation

scitools-ci[bot]
Copy link
Contributor

@scitools-ci scitools-ci bot commented Sep 15, 2022

Lockfiles updated to the latest resolvable environment.

If the CI tasks fail, create a new branch based on this PR and add the required fixes to that branch.

@scitools-ci scitools-ci bot added the Bot A bot generated issue/pull-request label Sep 15, 2022
@scitools-ci scitools-ci bot force-pushed the auto-update-lockfiles branch from 0cdf117 to e5ec4a8 Compare September 15, 2022 12:56
@bjlittle
Copy link
Member

⚡ wow!

@bjlittle
Copy link
Member

Oof!

2022-09-15T13:00:46.0903981Z make[1]: *** [Makefile:45: html] Segmentation fault (core dumped)

@trexfeathers
Copy link
Contributor

I am at least happy with the changes themselves - no unexpected packages etc.

@trexfeathers
Copy link
Contributor

trexfeathers commented Sep 15, 2022

The failure is rooted in: SciTools/cartopy@8860a81 - this change has been planned for several years, we just never spotted the warning (see #4384). It was finally implemented in SciTools/cartopy@fcb784d.

The change makes the below expected_mask incorrect. It was created as a 'snapshot' rather than an explicitly desired outcome, so should be fine to change it; will take a look now.

# Snapshot of mask with fixed tolerance of atol=2e-3
expected_mask = np.array(
[
[1, 1, 1, 0, 0, 0, 0, 0, 0, 1],
[1, 1, 1, 0, 0, 0, 0, 0, 0, 1],
[1, 1, 1, 1, 0, 0, 0, 0, 1, 1],
[1, 1, 1, 1, 0, 0, 0, 0, 1, 1],
[1, 1, 1, 1, 0, 0, 0, 0, 1, 1],
[1, 1, 1, 1, 1, 0, 0, 1, 1, 1],
[1, 1, 1, 1, 1, 0, 0, 1, 1, 1],
[1, 1, 1, 1, 1, 0, 0, 1, 1, 1],
],
np.bool_,
)

@trexfeathers
Copy link
Contributor

trexfeathers commented Sep 15, 2022

It was created as a 'snapshot' rather than an explicitly desired outcome, so should be fine to change it; will take a look now.

I'm no longer confident to make a change. This is all about testing whether masking removes deltas above a certain threshold, and with the underlying Cartopy changes this is no longer true. Drastically reducing the tolerance from 2e-3 to 2e-5 allows the below test to pass, but if the test sample size is increased it fails again.

self.assertTrue(anom[~ut.data.mask].max() < 0.1)

The below statement sounds like the original tolerance was a shot in the dark somewhat, and reading the conversations in the original PR - #1546 - backs this up.

# A value of atol=2e-3 is used, which corresponds to a change in magnitude
# of approximately 0.1%.

What is the failing test achieving? Just monitoring that the behaviour remains the same? If so what should the expected results be changed to? Should the masking tolerance also be changed?

@trexfeathers
Copy link
Contributor

trexfeathers commented Sep 15, 2022

Note that the following change allows all tests to pass, but is basically pinning us to old behaviour instead of adopting Proj's latest.

diff --git a/lib/iris/coord_systems.py b/lib/iris/coord_systems.py
index 802571925..ce00746ee 100644
--- a/lib/iris/coord_systems.py
+++ b/lib/iris/coord_systems.py
@@ -736,10 +736,10 @@ class OSGB(TransverseMercator):
         )
 
     def as_cartopy_crs(self):
-        return ccrs.OSGB()
+        return ccrs.OSGB(True)
 
     def as_cartopy_projection(self):
-        return ccrs.OSGB()
+        return ccrs.OSGB(True)

@trexfeathers trexfeathers mentioned this pull request Sep 16, 2022
3 tasks
@trexfeathers
Copy link
Contributor

Superceded by #4968.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bot A bot generated issue/pull-request New: Pull Request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants