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

Lockfile updates #4968

Merged
merged 9 commits into from
Sep 16, 2022
Merged

Lockfile updates #4968

merged 9 commits into from
Sep 16, 2022

Conversation

trexfeathers
Copy link
Contributor

@trexfeathers trexfeathers commented Sep 16, 2022

🚀 Pull Request

Description

  • All lockfiles updated (from [iris.ci] environment lockfiles auto-update #4967).
  • Changes to a test based on changes in Cartopy v0.21.0:
    • The specific change is Cartopy adopting a new default Transverse Mercator projection - changed in SciTools/cartopy@fcb784d, which was planned since SciTools/cartopy@8860a81. Read those for more context.
    • Our test was written to alert us to any changes in behaviour, rather than to enforce a 'correct' answer. @pp-mo, @bjlittle and I have reviewed the changed behaviour and accepted it by changing the expected test result.
    • The test 'area' has been also moved West to better demonstrate the intended masking behaviour, as the original area was showing very few masks since the Cartopy change.
  • NetCDF4 !=1.6.1 pin to avoid segfaults.

TODO:

  • What's New
  • Minimum Cartopy pin (>=0.21)
  • Diagnose intermittent GHA worker crashes

Consult Iris pull request check list

@trexfeathers
Copy link
Contributor Author

trexfeathers commented Sep 16, 2022

The intermittent HDF5 errors and GHA worker crashes appear associated with the same tests - those that read NetCDF files. I'm certain they are dependency related as I have simultaneously re-run another GHA run with our existing lockfiles (run 3060581803) and those all passed.

My guess is either the netCDF4 or NumPy changes.

I can't reproduce locally.

@trexfeathers
Copy link
Contributor Author

@trexfeathers trexfeathers marked this pull request as ready for review September 16, 2022 12:31
@bjlittle bjlittle self-assigned this Sep 16, 2022
Copy link
Member

@bjlittle bjlittle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trexfeathers Brilliant detective work 🕵️‍♂️

A couple of super minor format changes suggested (to align with existing formatting), otherwise let's get this banked 👍

requirements/ci/py310.yml Outdated Show resolved Hide resolved
requirements/ci/py38.yml Outdated Show resolved Hide resolved
requirements/ci/py39.yml Outdated Show resolved Hide resolved
@bjlittle bjlittle merged commit de911ce into SciTools:main Sep 16, 2022
@tinyendian
Copy link
Contributor

Hi @trexfeathers and @bjlittle, I had come across the test failures in test_rotate_winds.py that were addressed in this pull request while working on issue #4972.

If I am not mistaken, the code in iris.analysis.cartography._transform_distance_vectors_tolerance_mask essentially estimates the matrix norm ||A|| of a rotation matrix A that represents the actions of function _transform_distance_vectors, with

||A|| = inf/sup{ ||Ax|| for all ||x|| = 1}

where ||x|| is the magnitude of vector x, to find and mask cases with ||A|| < 0.999 or ||A|| > 1.001 where vector magnitudes are stretched or shrunken beyond a threshold of 0.1%.

At the moment, only two vector directions are tested, x=(1,0) and x=(0,1), which doesn't seem to capture the matrix norm sufficiently to identify all cases that fall outside the threshold.

The situation improves when one adds additional directions using something like

    ones = np.ones(x.shape)
    mask = np.logical_and(ones,ones)
    for angle in np.linspace(0, 0.5*np.pi, num=4):
        u_t, v_t = _transform_distance_vectors(np.cos(angle)*ones, np.sin(angle)*ones, ds, dx2, dy2)
        mag_sq = u_t**2 + v_t**2
        mask = np.logical_and(
            mask,
            np.isclose(mag_sq, ones, atol=2e-3),
        )
    mask = np.logical_not(mask)

For the failing unit test, unmasked vectors are now very close to the 0.1% threshold. The downside is that it is evidently a bit more expensive to compute, and the number and range of angles are free parameters - 4 angles between 0 and 90 degrees work in this case, but it's not immediately clear that this will suffice in others.

Not sure how critical it is for rotated wind vector magnitudes to stay close to the stretching threshold, but just in case you are interested in looking at that.

@trexfeathers
Copy link
Contributor Author

Thanks very much @tinyendian, it's good to have this information here; this is a topic that the team struggle to understand fully. We're happy with the situation as it is currently, but if we ever need to come back we can use the wisdom you've provided here 🙂

@pp-mo pp-mo mentioned this pull request Sep 26, 2022
trexfeathers added a commit to trexfeathers/iris that referenced this pull request Sep 26, 2022
* Updated environment lockfiles

* Adjustments for Cartopy v0.21.0 (SciTools/cartopy@fcb784d).

* Cartopy >=0.21 pin.

* What's New entry.

* WIP try netCDF4 pin.

* Try pip installing netcdf4==1.6.1 as requested by @ocefpaf.

* Revert "Try pip installing netcdf4==1.6.1 as requested by @ocefpaf."

This reverts commit ce9f890.

* netcdf4!=1.6.1

* Align Conda YAML formatting.

Co-authored-by: Lockfile bot <noreply@github.com>
lbdreyer pushed a commit that referenced this pull request Sep 26, 2022
* Lockfile updates (#4968)

* Updated environment lockfiles

* Adjustments for Cartopy v0.21.0 (SciTools/cartopy@fcb784d).

* Cartopy >=0.21 pin.

* What's New entry.

* WIP try netCDF4 pin.

* Try pip installing netcdf4==1.6.1 as requested by @ocefpaf.

* Revert "Try pip installing netcdf4==1.6.1 as requested by @ocefpaf."

This reverts commit ce9f890.

* netcdf4!=1.6.1

* Align Conda YAML formatting.

Co-authored-by: Lockfile bot <noreply@github.com>

* New lockfiles.

* What's New entry.

* What's New correction.

Co-authored-by: Lockfile bot <noreply@github.com>
@trexfeathers trexfeathers deleted the lockfile_updates branch October 12, 2022 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants