Update MeshXY's .filters method to work with coordinates that do not have a standard name and allow the mesh to be saved#7185
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7185 +/- ##
=======================================
Coverage 90.15% 90.15%
=======================================
Files 91 91
Lines 24989 24990 +1
Branches 4687 4686 -1
=======================================
+ Hits 22528 22529 +1
Misses 1683 1683
Partials 778 778 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…xis (allows for axis without a standard name set)
trexfeathers
left a comment
There was a problem hiding this comment.
This looks lovely!
I imagine you're looking into the test failures. I don't know what to make of the docs failure though; I'll look into that.
| attributes=None, | ||
| axis=None, | ||
| item: str | None | Any = None, | ||
| standard_name: str | None = None, |
There was a problem hiding this comment.
You know more than I do in this space. Is Optional[str] frowned upon, or a matter of personal choice?
There was a problem hiding this comment.
It's mostly a personal choice and it's also best practice: https://typing.python.org/en/latest/reference/best_practices.html#shorthand-syntax . It's easier to write and doesn't need to import something. There has been some discussion I've also seen discussions where they could have a different semantic meanings: https://stackoverflow.com/a/73591412 (not that I agree with them)
It might be worth adding something to a style guide if this is a distinction worth having though.
| var_name=None, | ||
| attributes=None, | ||
| axis=None, | ||
| item: str | None | Any = None, |
There was a problem hiding this comment.
| item: str | None | Any = None, | |
| item: str | None | _DimensionalMetadata | BaseMetadata = None, |
| A dictionary of attributes desired on the object. If ``None``, | ||
| does not check for ``attributes``. | ||
| attributes : Mapping, optional, Any | ||
| A mapping of attributes desired on the object. If ``None``, |
There was a problem hiding this comment.
My suggestion is less technically pure, but it would be helpful to find a way to mention dict in the phrasing here. Plenty of our users don't know what a Mapping is.
There was a problem hiding this comment.
Ah that's fair. I'll add it as part of the comments. I actually can't remember why I changed it from a dict to a Mapping. I have a feeling mypy was shouting at me when somewhere else a mapping was created and I had to change it for that.
| attributes=None, | ||
| axis=None, | ||
| location=None, | ||
| item: str | object | None = None, |
There was a problem hiding this comment.
| item: str | object | None = None, | |
| item: str | None | _DimensionalMetadata | BaseMetadata = None, |
| attributes : dict, optional | ||
| A dictionary of attributes desired on the coordinates. If ``None``, | ||
| attributes : Mapping, optional | ||
| A mapping of attributes desired on the coordinates. If ``None``, |
There was a problem hiding this comment.
Again, some way to communicate to novice users that Mapping covers dict.
| attributes=None, | ||
| axis=None, | ||
| location=None, | ||
| item: str | None | Any = None, |
There was a problem hiding this comment.
| item: str | None | Any = None, | |
| item: str | None | _DimensionalMetadata | BaseMetadata = None, |
|
Explanation for the docs failure: #7191 (comment) Our docs builds on Readthedocs are vulnerable because we have not yet managed to lock the dependency versions there - unlike all our tests, which is why the rest of the CI on this PR is still fine. See SciTools/workflows#38 |
Description
closes #4863
In investgating the issue, we found that when saving a
Meshand converting them toMeshCoords had root a root problem inMesh.filters(), where it was ultimately trying to guess the coord's axis, which relies on thestandard_name.This pull request addresses the issue by adding
mesh_filters, which directly pulls the coordinates out of the mesh, without needing to consider their names. The construction of the mesh already gave us the information needed to do this. The originalfiltersfunction callsmesh_filtersbefore sending it to themetadata_filterlike before, but removing the axis from the call because it is already accounted for.To make the filter slightly more intelligent and to avoid the
hasattr(self, "face_coords")line,_Mesh2DCoordinateManagerimplements its ownmesh_filtersto deal with face coordinates separately. It also gave the added benefit of raising aValueErrorspecific to the_Mesh1DCoordinateManagercase, where it mentions that it only expects node and edge locations (not face). However, this has broken tests, so I will need to check if this causes any unexpected issues for users.In the case for saving coordinates without a
standard_name, we decided that naming it as "unknown" should be suffice, like what is done for cubes. In the case of using ncdump with the MWE:There may be some way to give a more meaningful name, but it will involve making assumptions which may not apply to everyone.
I have tried my hand at adding types as I go along, but most areas are beyond my knowledge. Ultimately, I would like to remove the
Anytypes I added to make mypy happy, but will take more time for me to know how to best update them.Checklist
Important
The Iris core developers are here to help! If anything below is unclear, just post a comment asking for help 😊
(further reading)
Tip
Things you can trigger on this PR:
9999for this PR's number - to re-trigger the CLA check:https://cla-assistant.io/check/SciTools/iris?pullRequest=9999