UGRID Loading (AVD-1656, AVD-1657, AVD-1660)

[trexfeathers]

🚀 Pull Request

Description

This PR implements correct parsing of UGRID content in NetCDF files, creating our new data model objects and attaching the resulting MeshCoords to the loaded cube(s).

The functionality is provided in iris.experimental.ugrid, making it an optional alternative to the standard functionality, and making it clear that it is subject to rapid change.

How it works

The user accesses the alternative functionality by wrapping their loading calls in a context manager - experimental.ugrid.parse_ugrid_on_load() - which instructs the loading internals to not use netcdf.load_cubes(), but instead use an enhanced UGRID-aware version - experimental.ugrid.load_cubes().

This allows us to offer all the existing loading goodness without needing to write UGRID-versions of most of the code. I can confirm from rough testing that users can still use constraints and callbacks on the non-UGRID elements of the file. If there are elements that don't work with the UGRID file elements (e.g. a constraint on latitude), that's fine since it's still experimental and we haven't yet promised such functionality.

Re-integration

To achieve good isolation in experimental.ugrid, I have sub-classed/replicated elements from fileformats.netcdf and fileformats.cf. But I originally wrote my code within the existing structure as a PoC, so I hope this means there should be little trouble when we eventually fold experimental.ugrid into Iris' standard functionality.

To do

• Mesh.toMeshCoord() unit tests
• netcdf.load_cubes() unit tests
• loading integration tests (including an iris-test-data PR)

---

Consult Iris pull request check list

[trexfeathers]

@pp-mo I've responded to your suggestions from our offline conversations.

Still got some more tests to add (see main description).

[trexfeathers]

I consider this now done, excepting:

Successful CI tests will require the merging of SciTools/iris-test-data#59, then an update to the Cirrus IRIS_TEST_DATA_REF.

@bjlittle / @jamesp : following #4024 should we actually be creating a new release of iris-test-data and referencing that?

[trexfeathers]

Ready to go @pp-mo.

There's a single broken link check but having looked I think that might come back if re-run later.

[pp-mo]

Not much to worry about here.
I'm sending this now, and I think that is mostly it for the main code.
But I haven't checked out any much of the tests yet, so I intend to add another review after.

[pp-mo]

Hi @trexfeathers here's my remaining suggestions, all about the testing this time.
Nothing major, as you might expect !

In addition though, I must say I am rather dispirited at the extent of boilerplate copying in the tests for the new ugrid-specific CFVariable dataclasses, specifically the various 'identify' methods.
But after some reflection, I have thoroughly convinced myself that this is just not your "fault" (!), and this is not the time to address it.

[pp-mo]

I am rather dispirited at the extent of boilerplate copying in the tests for the new ugrid-specific CFVariable dataclasses, specifically the various 'identify' methods.

There are several reasons for this IMHO :

• the original code has no testing of these features (!!)
• the original code has extensively copied-and-modified sections which resist generalising
• and arguably, the CF spec itself noticeably fails to generalise its concepts, and uses much special-casing, which obviously has made it really hard to write this code in a more integrated and constrained DRY style.

So, I was just generally "complaining" about that really, and I don't think it is appropriate to ask for any further improvements here as it would mean a lot of refactoring.
I think I will however create an issue covering whether we could usefully refactor this to improve structure for the future.

[trexfeathers]

I am rather dispirited at the extent of boilerplate copying in the tests for the new ugrid-specific CFVariable dataclasses, specifically the various 'identify' methods.

@pp-mo I appreciate what you're saying. IMO it's a problem that the existing CFVariable subclasses didn't have specific tests when they each implemented their own .identify() methods. Perhaps if someone had written such tests they would have realised that it would be better to abstract a single CFVariable.identify() method, thus simplifying the testing requirements.

I would have preferred a more abstracted solution, but my implementation has been driven by three things:

• It would be bad practice to introduce these new classes without writing tests - "start as you mean to go on".
• It would be bad practice to write a common test to cover three different .identify() methods - there's no contract that says they should all continue to behave the same.
• It would be bad practice to significantly re-work the existing CFVariable subclasses to support a new pattern that is currently in iris.experimental.

Anyway I'll stop moaning since it sounds like you agree with me 😆

[trexfeathers]

Fabulous, thanks @pp-mo!