Skip to content

Conversation

pp-mo
Copy link
Member

@pp-mo pp-mo commented Nov 20, 2019

Load ancils for netcdf.
STATUS: why this targets the 'launch_ancils' feature branch
We aren't yet sure that we will be ready to "release" ancillary variables in Iris 3.0
(mostly because there are a lot of places where specific handling is needed to avoid creating bad cubes).
Hence this is the key PR on the launch_ancils feature branch.

In practice, some support has already been merged to master.
Especially #3422, which also already makes the cube API for ancils visible.
However.. bad things don't happen until cubes contain ancils, which will generally only happen to users when we merge this PR, following which they can be generated when loading.
Likewise, we are also "holding back" on other PRs, targetting the feature branch instead of master), e.g. documentation additions


NOTES:

  • also silently fixes an hitherto unknown problem, whereby attributes of cell-measures were not loaded.
    • possibly there should be an extra, specific test for that ? But I think it is not really needed.
  • only one test, at present

@pp-mo pp-mo added this to the v3.0.0 milestone Nov 20, 2019
@abooton abooton changed the title Netcdf loading ancillary variables P.3473: Netcdf loading ancillary variables Nov 26, 2019
@abooton abooton changed the title P.3473: Netcdf loading ancillary variables P3473: Netcdf loading ancillary variables Nov 26, 2019
@abooton abooton changed the title P3473: Netcdf loading ancillary variables PI3473: Netcdf loading ancillary variables Nov 26, 2019
@abooton abooton changed the title PI3473: Netcdf loading ancillary variables PI-3473: Netcdf loading ancillary variables Nov 26, 2019

self.assertCML(cubes, ("netcdf", "netcdf_cell_methods.cml"))

def test_ancillary_variables(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

It might also be worthwhile also testing for cell measures. As far as I can tell, they aren't currently being tested for.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok..

Copy link
Contributor

@stephenworsley stephenworsley left a comment

Choose a reason for hiding this comment

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

Looks good, I've convinced myself this treats cell measures and ancillaries in a way that is consistent with current treatment of coordinates. Just a couple very minor things to look at for tidiness sake.

attr_units = get_attr_units(cf_cm_var, attributes)

data = _get_cf_var_data(cf_cm_attr, engine.filename)
data = _get_cf_var_data(cf_cm_var, engine.filename)
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, should this have a comment similar to this line?

# Get (lazy) content array
data = _get_cf_var_data(cf_av_var, engine.filename)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, but "consistency" is a bit of a lost cause in this code IMHO. Not least because we have far too much code duplication code, as here !
( N.B. We do still hope to replace all of this with Python code #3415 )

coord.var_name: coord.standard_name or coord.var_name or "unknown"
for coord in cube.coords()
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Should there still be whitespace here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I suppose so.
I might have hoped that Black would take care of this, but clearly the results are not totally standardised ..

@stephenworsley stephenworsley self-assigned this Dec 4, 2019
@pp-mo
Copy link
Member Author

pp-mo commented Dec 9, 2019

Thanks @stephenworsley
Hopefully addressed your previous comments now.
Please re-review.

Copy link
Contributor

@stephenworsley stephenworsley left a comment

Choose a reason for hiding this comment

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

Looking good, just one more small change needed.

my_areas:custom = "extra-attribute";
data:
axv = 11, 12, 13;
ayv = 21, 22, 23;
Copy link
Contributor

Choose a reason for hiding this comment

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

ayv should be length 2.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks.
I'm surprised it didn't error that!

@pp-mo
Copy link
Member Author

pp-mo commented Dec 12, 2019

Hi @stephenworsley
Fixed ?

@stephenworsley
Copy link
Contributor

@pp-mo Looks good. I think this just needs rebasing to make Travis pass

Copy link
Contributor

@stephenworsley stephenworsley left a comment

Choose a reason for hiding this comment

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

Looks good, ready to merge!

@stephenworsley stephenworsley merged commit 7703699 into SciTools:launch_ancils Dec 13, 2019
abooton pushed a commit to abooton/iris that referenced this pull request Jun 5, 2020
* _regrid_area_weighted_array: Tweak variable order to near other use in code (SciTools#3571)

* Fix problems with export and echo command. (SciTools#3577)

* Pushdocs fix2 (SciTools#3580)

* Revert to single-line command for doctr invocation.

* Added script comment, partly to force Github respin.

* Added whatsnew for Black. (SciTools#3581)

* Fixes required due to the release of iris-grib v0.15.0 (SciTools#3582)

* Fix python-eccodes pin in travis (SciTools#3593)

* Netcdf load of ancillary vars: first working.
stephenworsley added a commit to stephenworsley/iris that referenced this pull request Aug 20, 2020
… launch_ancils

* 'launch_ancils' of https://github.com/SciTools/iris:
  Resolve conflicts with requirements/test.txt to update the imagehash pin to imagehash>=4.0 (SciTools#3735)
  PI:3358 (WIP) Ensure flags load/save without units (SciTools#3613)
  PI-3473 Rename "engine.provides" (SciTools#3590)
  PI-3473: Whatsnews relating to ancillary load + save. (SciTools#3557)
  PI-3473: Netcdf loading ancillary variables (SciTools#3556)

� Conflicts:
�	lib/iris/fileformats/netcdf.py
�	lib/iris/tests/unit/fileformats/netcdf/test__load_aux_factory.py
�	requirements/core.txt
@pp-mo pp-mo deleted the ancil_load branch March 18, 2022 15:27
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