Allow passing None for all coord-system optional args. #3804

pp-mo · 2020-08-25T18:11:37Z

The need for this is due to the way the netcdf load rules translate the attributes of a grid-mapping variable into a coordinate system : The functions "build_xxx_coordinate_system" (in the Pyke rules, e.g. here) fetch netcdf var attributes defaulted to None, and then call the class constructor expecting it to fill in defaults for unspecified arguments. But this cannot work if the constructor relies on an optional argument definition of the form "arg=number", e.g. here :
So instead, the CoordSystem constructors should be capable of accepting an "arg=None" and applying a default.

Aiming at:
closes #3629
closes #3675

…stems.

trexfeathers

Thanks @pp-mo, this is a whopper! Definitely takes the record for the largest PR I have reviewed. I'm particularly impressed that - despite what I assume was a lot of copy-paste - each of the tests refers to the correct variables and the correct defaults. And despite the large number of changed coordinate systems almost everything had the right corresponding tests, although see the note below. Top work! 🏆 💐

Further to my specific code comments, here are some notes on possible missing coverage for the pyke rules tests:

Missing equivalent test coverage for:
- fc_rules_cf_fc.build_coordinate_system
- fc_rules_cf_fc.build_rotated_coordinate_system
Not sure why there is no fc_rules_cf_fc.build_orthographic_coordinate_system?

trexfeathers · 2020-09-09T15:54:51Z

lib/iris/coord_systems.py

            * false_easting
                    X offset from planar origin in metres.
+                    Defaults to 0.


Is there a reason you're not saying Defaults to 0.0. for consistency?

Still at:

L980

L983

L1076

lib/iris/coord_systems.py

lib/iris/tests/unit/coord_systems/test_AlbersEqualArea.py

lib/iris/tests/unit/coord_systems/test_GeogCS.py

...s/pyke_rules/compiled_krb/fc_rules_cf_fc/test_build_transverse_mercator_coordinate_system.py

...fileformats/pyke_rules/compiled_krb/fc_rules_cf_fc/test_build_verticalp_coordinate_system.py

docs/iris/src/whatsnew/latest.rst

pp-mo · 2020-09-11T10:42:29Z

why there is no fc_rules_cf_fc.build_orthographic_coordinate_system?

It looks like we just don't support this for netcdf load.
A lot of the confusion here is generated by the differences between what netcdf currently supports and the generic iris coord-systems code.
Which only exacerbates a lot of tiresome nearly-but-not-quite-the-same-ness in the CF rules.

pp-mo · 2020-09-11T11:03:43Z

Missing equivalent test coverage for:

fc_rules_cf_fc.build_coordinate_system

fc_rules_cf_fc.build_rotated_coordinate_system

These are not affected by the primary code changes, because they do not support false_easting/northing parameters : they only exist in non-latlon systems.
So I think it's certainly true there is testing missing here, but it's historical and we are not obliged to address it now.

Frankly, there is a lot that is untidy and missing in the existing testing strategies, for both the iris coord-systems, and the netcdf loading. All the tests in iris.tests which are neither under iris.tests.unit nor iris.tests.integration, are effectively outdated, for a start.
I have explicitly opted for a "minimum changes" approach here, providing only the needed new tests, which I think should be less confusing, and hopefully less work, for both of us : Whilst it might actually have been simpler to rationalise the existing stuff first, I thought it looked like a lot of work, and addressing the missing parts was particularly likely to cause overcomplication.

P.S. I just noticed this, prior art which relates : #3409

pp-mo

Initial comments -- not done yet ...

docs/iris/src/whatsnew/latest.rst

lib/iris/coord_systems.py

lib/iris/tests/unit/coord_systems/test_AlbersEqualArea.py

lib/iris/tests/unit/coord_systems/test_Geostationary.py

lib/iris/tests/unit/coord_systems/test_Stereographic.py

.../fileformats/pyke_rules/compiled_krb/fc_rules_cf_fc/test_build_mercator_coordinate_system.py

…graphic.true_scale_lat

trexfeathers

Thanks for the hard work @pp-mo , I consider my comments largely addressed, with the exception of the two new comments below.

lib/iris/tests/unit/coord_systems/test_VerticalPerspective.py

trexfeathers · 2020-09-14T10:48:51Z

lib/iris/coord_systems.py


            * false_easting
-                    X offset from planar origin in metres.
+                    X offset from planar origin in metres.  Defaults to 0


@pp-mo and I have agreed offline that this file needs a quick check through to make sure docstrings consistently make it clear that the defaults are float types, since different lines containing integers and floats may be misinterpreted as actual different behaviour.

Co-authored-by: Martin Yeo <[email protected]>

pp-mo · 2020-09-14T16:35:56Z

I decided in the end to rationalise all the docstrings,
so I hope this suits.
I have tried to put them all into the existing iris-format, as even though we may be close to adopting something different #3840, I think we aren't totally decided on numpy style yet.

I also considered adding types for all the instance parameters, e.g. #: X offset from planar origin in metres (float)., but it seemed fussy + unnecessary (and we don't have a definite style for it anyway).

lib/iris/coord_systems.py

trexfeathers · 2020-09-15T08:46:19Z

Great work @pp-mo 👌 LGTM

…s#3838.

* master: whatsnew - using the author github id (SciTools#3849) Bring SciTools#3804 whatsnew in line with format changes from SciTools#3838. (SciTools#3847) "What's New" credit and referencing (SciTools#3838) Allow passing None for all coord-system optional args. (SciTools#3804) var_name in AuxCoord created by trajectory.interpolate() (SciTools#3718)

pp-mo force-pushed the optional_crs_args branch from 3643e2b to 9ef63e5 Compare August 25, 2020 18:42

bjlittle added the Type: Bug label Sep 7, 2020

bjlittle added this to the v3.0.0 milestone Sep 7, 2020

pp-mo self-assigned this Sep 8, 2020

Allow passing None for all coord-system optional args.

57a53ac

pp-mo force-pushed the optional_crs_args branch from 9ef63e5 to 57a53ac Compare September 8, 2020 16:50

pp-mo added 3 commits September 8, 2020 18:11

Tests for coord-system default args behaviours.

5c0edaa

CML fixes for detail changes to coord-systems.

f16b158

Tests for netcdf load with no false_easting/northing, all relevant sy…

71b59d1

…stems.

trexfeathers self-requested a review September 9, 2020 10:18

Add whatsnew entry.

059ef6e

pp-mo force-pushed the optional_crs_args branch from 059ef6e to 17485d2 Compare September 9, 2020 14:21

Merge branch 'latest' into optional_crs_args_REDO2

ad14497

pp-mo force-pushed the optional_crs_args branch from 17485d2 to ad14497 Compare September 9, 2020 14:38

trexfeathers requested changes Sep 9, 2020

View reviewed changes

Fix matplotlib capitalisation : conflict resolution error.

55d032c

pp-mo commented Sep 11, 2020

View reviewed changes

pp-mo added 4 commits September 11, 2020 18:43

Handle optional None and parallels in _arg_default. Fix bug in Stereo…

7f1dd13

…graphic.true_scale_lat

Rationalise coord_system optional arg tests.

eaf5e7b

Fix arg_default parallels.

77a7d2c

Fix exception tests for too-many/too-few parallels.

7dadfdb

trexfeathers requested changes Sep 14, 2020

View reviewed changes

pp-mo and others added 2 commits September 14, 2020 15:38

Update lib/iris/tests/unit/coord_systems/test_VerticalPerspective.py

1287dfd

Co-authored-by: Martin Yeo <[email protected]>

Normalise all coord-system docstrings.

3de9b24

pp-mo commented Sep 14, 2020

View reviewed changes

lib/iris/coord_systems.py Show resolved Hide resolved

pp-mo commented Sep 14, 2020

View reviewed changes

lib/iris/coord_systems.py Show resolved Hide resolved

pp-mo commented Sep 14, 2020

View reviewed changes

lib/iris/coord_systems.py Show resolved Hide resolved

trexfeathers approved these changes Sep 15, 2020

View reviewed changes

trexfeathers merged commit 3e9a094 into SciTools:master Sep 15, 2020

trexfeathers added a commit to trexfeathers/iris that referenced this pull request Sep 15, 2020

Bring SciTools#3804 whatsnew in line with format changes from SciTool…

ad817ff

…s#3838.

trexfeathers mentioned this pull request Sep 15, 2020

Bring #3804 "What's New" in line with format changes from #3838. #3847

Merged

bjlittle pushed a commit that referenced this pull request Sep 15, 2020

Bring #3804 whatsnew in line with format changes from #3838. (#3847)

6afb6db

pp-mo mentioned this pull request Oct 1, 2020

Fix iris3 coord units SciTools/iris-grib#214

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow passing None for all coord-system optional args. #3804

Allow passing None for all coord-system optional args. #3804

pp-mo commented Aug 25, 2020 •

edited

Loading

trexfeathers left a comment

trexfeathers Sep 9, 2020

pp-mo Sep 11, 2020

trexfeathers Sep 14, 2020

pp-mo commented Sep 11, 2020

pp-mo commented Sep 11, 2020 •

edited

Loading

pp-mo left a comment

trexfeathers left a comment

trexfeathers Sep 14, 2020

pp-mo commented Sep 14, 2020 •

edited

Loading

trexfeathers commented Sep 15, 2020

Allow passing None for all coord-system optional args. #3804

Allow passing None for all coord-system optional args. #3804

Conversation

pp-mo commented Aug 25, 2020 • edited Loading

trexfeathers left a comment

Choose a reason for hiding this comment

trexfeathers Sep 9, 2020

Choose a reason for hiding this comment

pp-mo Sep 11, 2020

Choose a reason for hiding this comment

trexfeathers Sep 14, 2020

Choose a reason for hiding this comment

pp-mo commented Sep 11, 2020

pp-mo commented Sep 11, 2020 • edited Loading

pp-mo left a comment

Choose a reason for hiding this comment

trexfeathers left a comment

Choose a reason for hiding this comment

trexfeathers Sep 14, 2020

Choose a reason for hiding this comment

pp-mo commented Sep 14, 2020 • edited Loading

trexfeathers commented Sep 15, 2020

pp-mo commented Aug 25, 2020 •

edited

Loading

pp-mo commented Sep 11, 2020 •

edited

Loading

pp-mo commented Sep 14, 2020 •

edited

Loading