Conversation
…lti-dim dim-coords.
…olled by a table.
… the cube itself.
pp-mo
force-pushed
the
cube_print_respin
branch
from
564a746 to
f40aadc
Compare
|
Hi @trexfeathers |
@pp-mo I'm on it now 👀 sorry for the delay. |
|
Note to self: have so far reviewed up to and including: 988710c |
trexfeathers
left a comment
trexfeathers
left a comment
There was a problem hiding this comment.
@pp-mo this looks like a huge improvement! 💐
A few things for you to address/answer, mainly on test coverage. Thanks!
lib/iris/cube.py
Outdated
| scalar_coords.sort(key=lambda coord: coord.name()) | ||
| # Sort vector coordinates by data dimension and name. | ||
| vector_dim_coords.sort( | ||
| key=lambda coord: (self.coord_dims(coord), coord.name()) |
There was a problem hiding this comment.
| key=lambda coord: (self.coord_dims(coord), coord.name()) | |
| key=lambda coord: (self.coord_dims(coord)) |
Isn't this redundant, since it's not possible to have multiple DimCoords against a single dimension?
There was a problem hiding this comment.
Another good one!
I think, like the former code to select dim-names from multiple dim-coords, which I already removed, this is really an ancient legacy of a time when we hadn't yet sorted out what was and was not allowed for dim-coords.
It goes back to the initial commit on GitHub
(!)
There was a problem hiding this comment.
@pp-mo has modified but not certain we're there yet. See: #3914 (comment)
|
Many thanks @trexfeathers . You seem to have done a very thorough job here ! 🚀 |
|
@trexfeathers But in retrospect, that may be a useful reference idea, but it is over-strict : |
lib/iris/cube.py
Outdated
| vector_aux_coords.sort(key=self.coord_dims) | ||
| vector_derived_coords.sort(key=self.coord_dims) |
There was a problem hiding this comment.
I don't actually know about derived_coords, but I was fairly certain that you are allowed to have multiple aux_coords against a single dimension, so it's still appropriate to keep the second sort by coord.name(). Am I wrong?
There was a problem hiding this comment.
We have now discovered that all 5 of the components in section_specs are intrinsically sorted since that's the way they come out of the Cube! @pp-mo is going to remove the sort lines.
| ], | ||
| ) | ||
|
|
||
| def test_aux_coords_ordering(self): |
There was a problem hiding this comment.
Would you need the same for derived_coords?
There was a problem hiding this comment.
Having discussed with @pp-mo, we've agreed that all we need to do is convert this test into a general confirmation of correct sorting, by:
- Changing the test name
- Adding a few
dim_coords
|
Since @trexfeathers prompting above, I have realised that none of the element sorting is actually needed, as all the relevant cube access methods already provide the results ready-sorted by (cube-dims, name), which is exactly what we wanted. So, I have now removed all that sorting code. Also I have now rationalised all the scalar-element testing : "Scalar" elements are now consistently those with an empty " ALSO, I've added a possible "scalar ancillary variables" section, which was previously missing. |
|
So long as the tests pass, I will be happy with this..! |
3 participants
Now with no specific reference to ugrid.
This is now an extensive refactor of the Iris Cube
summaryand_repr_html_methods.Including changes needed to enable the forthcoming iris-ugrid
UCubes to produce "extended" cube summmaries.I have added testing for the new factored-out helper methods. But I haven't provided any test of how modifying these can produce extended cube summaries, as its not really an "API" + the requirement is hard to generalise.
That usage will eventually get tested by the specifics in iris-ugrid.
Meanwhile, you should see that all the existing tests simply pass. 🏹
Proposals for the specific use of this in iris-ugrid are here : SciTools-incubator/iris-ugrid#26