Skip to content

Conversation

@pp-mo
Copy link
Member

@pp-mo pp-mo commented Sep 15, 2021

🚀 Pull Request

Description

Support for saving cubes with meshes.
(No "save_mesh" yet, easy to add that later)

Current status :

Good for review.

Some additional questions to consider ..

  • should we minimise the number of small/bounds dimensions created ? See this comment
  • should we be augmenting the global 'Conventions' attribute See this comment
  • should the cube-summary changes be pulled out into a separate PR ?

Consult Iris pull request check list

@pp-mo pp-mo force-pushed the ugrid_save branch 3 times, most recently from 9e3c978 to 9c63c3e Compare September 17, 2021 10:44
@pp-mo pp-mo marked this pull request as ready for review September 20, 2021 09:26
@pp-mo
Copy link
Member Author

pp-mo commented Sep 21, 2021

General question : should we be adding some kind of UGRID reference to the global 'Conventions' attribute ?

  • Iris currently just uses Conventions = "CF=1.7" : see here and here
  • LFRic/XIOS output files currently have Conventions = "UGRID" : see here
  • in pure netcdf terms, we can apparently have "CF UGRID", but there is no special mention of of meaning for versions : See here
  • there is no particular agreement within either CF or UGRID for how to do this, or refer to the other
    • i.e. does "UGRID" imply CF ? That doesn't seem to work as the versions are explicitly connected (at least yet)
    • there is discussion here but no clear winning solution yet

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.

@pp-mo Over to you. It's looking good, considering what you inherited 😉

A few review comments to service, but nothing major. I've still to sweep through the tests, so I'll do that in the meantime.

@pp-mo
Copy link
Member Author

pp-mo commented Sep 23, 2021

Ok, that was a useful stage to review both the existing code + my changes.
Some more tweaks, now feeling more confident.
@bjlittle I hope this addressed everything raised so far (but I pushed back on a couple) : please re-review

@pp-mo pp-mo mentioned this pull request Sep 24, 2021
@bjlittle bjlittle merged commit 9950bd7 into SciTools:main Sep 28, 2021
@bjlittle
Copy link
Member

@pp-mo Truly super stuff, thanks 🎉

@trexfeathers
Copy link
Contributor

I have 105 email notifications about this - you two must have worked really hard! 🏆🏆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants