Coord print #4499
Coord print #4499
Conversation
|
Sample results : |
|
I just tried this out with a file I had lying around. I think it's a big improvement as there is less mess all over the screen! I also really like that you include the shapes and dtype. Sometimes, though, it is useful to print the lot out in order to trawl through and figure out what's going wrong! Of course we can print the points and bounds separately and for time coordinates I was thinking I could do print(coord.units.num2date(coord.points))but that gave me this, which I think is slightly hideous: Is there a way the user could easily access the time-formatting used by the coord |
|
Never mind. I just realised I can do print(coord.units.num2date(coord.points).astype(str)) |
It's an interesting point, and I think there's still scope for a helper to do this. Another way would be to provide a utility equivalent to the |
Would that be a job for |
|
Mods trying out some newer ideas -- see #4501 (reply in thread) |
Having slept on it, how about something like |
|
some new tweaks. |
Still thinking about that, but let's make it a separate issue ? |
|
In case anyone doesn't get a ping from the Discussion, I just raised there an awkward question of column formatting ... |
|
( Note: rebased onto upstream/main ; removed some test-debug code ) Still TODO:
|
|
Finally done ! There is one possibly outstanding issue, regarding the printout of attributes... (separate comment follows) |
|
Currently I am just printing attributes as a repr of a dict, and "hoping" it fits on a line @lbdreyer already suggested that this might be nasty in some cases (like long comment or history-type attributes) |
lib/iris/coords.py
Outdated
| max_array_width=None, | ||
| ): | ||
| """ | ||
| Make a printable text summary of a dimensional cube component. |
There was a problem hiding this comment.
"dimensional cube component" may be a bit confusing. We don't use that term anywhere and it's a bit hard to parse.
Instead, you could leave it as:
Make a printable text summary.
As this is a method on the AuxCoord, DimCoord, etc it may be clear enough to the user what object it is summarising.
or you could incorporate the name of the class?
Make a printable text summary of the DimCoord.
There was a problem hiding this comment.
Not sure of creating the string dynamically. It might work in Sphinx, or not ?
I will have a try ...
There was a problem hiding this comment.
No, I think it's not possible to produce a different docstring for each subclass without a metaclass type of operation (post-definition), which I think is too much fuss.
I see we do have some operations which describe themselves as 'dimensional metadata'. E.G. file:https:///net/home/h05/itpp/git/iris/iris_main/docs/src/_build/html/generated/api/iris/coords.html?highlight=dimensional%20metadata#iris.coords.CellMeasure.copy
But I think just leaving it un-named is easier, as you suggest.
I have just done that for now a2a6015
lib/iris/coords.py
Outdated
| result += addline + f"shape: {shape_str}" | ||
|
|
||
| if self.has_bounds(): | ||
| # line = f'bounds_shape: {self._bounds_dm.shape}' |
There was a problem hiding this comment.
I presume you forgot to remove this?
| " long_name: 'enc'", | ||
| " cf_role: 'edge_node_connectivity'", | ||
| " start_index: 0", | ||
| " src_dim: 0", |
There was a problem hiding this comment.
I presume this will need to be updated once #4492 is merged in
There was a problem hiding this comment.
Yes, these will need changing in the MeshCoord.__str__ code as well.
Most other cases will happen 'automatically', as the generic code uses the metadata._fields to list the properties. But the relevant test results will still need fixing.
lib/iris/experimental/ugrid/mesh.py
Outdated
| # Modify the generic 'default-form' result to produce what we want. | ||
| if shorten: | ||
| # Single-line form : insert the mesh+location before the array part | ||
| i_array = result.index("[") |
There was a problem hiding this comment.
Perhaps the corneriest of corner cases but, if a long name included a "[" that would presumably cause problems here. Should we worry about that?
There was a problem hiding this comment.
Actually, the multiline case uses a search for 'location:', which in principle is also not solid.
I realised that I was repeating an old mistake here : this was the problem we had with the 'old' Cube.__str__ output, where repr_html was pulling it apart again and searching for certain pieces -- which was very fragile and troublesome.
So I have now added some crude (private) "structural information" output behaviour instead, to support the fiddling that MeshCoord.summary needs to do.
| self.assertRegex(result, re_expected) | ||
|
|
||
| def test_alternative_location_and_axis(self): | ||
| meshcoord = sample_meshcoord(mesh=self.mesh, location="edge", axis="y") | ||
| result = str(meshcoord) | ||
| re_expected = r", location='edge', axis='y'" | ||
| # re_expected = r", location='edge', axis='y'" |
There was a problem hiding this comment.
Were you planning to removed this?
lib/iris/coords.py
Outdated
| data_str = "[...]" | ||
|
|
||
| if self.has_bounds(): | ||
| data_str += "+bnds" |
There was a problem hiding this comment.
Can I persuade you to change this to
| data_str += "+bnds" | |
| data_str += "+bounds" |
It is only two characers after all, and I would prefer we use the official term "bounds" rather than invent a new term "bnds". Dropping the vowels may be natural and obvious to us but I would be concerned that this may trip some people up (non-native english speakers, perhaps?).
I think it's fine to use bnds to name a variable in some example code but not in part of the code in this way
For two vowels I would say it is worth being explicit
…nds repr when lazy.
| self.lat = iris.tests.stock.realistic_4d().coord("grid_latitude")[:10] | ||
| cube = iris.tests.stock.realistic_4d() | ||
| self.lat = cube.coord("grid_latitude")[:10] | ||
| self.height = cube.coord("level_height")[:10] |
There was a problem hiding this comment.
These tests were a bit of a mess : in both the 'time' and 'nontime' classes, the 4 cases were 2 duplicates of only 2 things.
Unfortunately I'm not sure I've improved things much, though.
'height' is not actually an AuxCoord (class), though it is an aux-coord in the cube.
likewise, 'forecast_period' is not a brilliant example of a "time AuxCoord".
Given that this wasn't doing anything very useful before, I'm not going to spend any more effort on it.
|
Many thanks @lbdreyer . |
Viewed from the morning after, I think the core routine is less clear now, and I'm sure that can be improved. |
Ok, I think that is a fair bit clearer ! |
| line("optional connectivities", 1) | ||
| for name, conn in optional_conns.items(): | ||
| conn_string = conn.summary(shorten=True, linewidth=0) | ||
| line(f"{name}: {conn_string}", 2) |
There was a problem hiding this comment.
Could there be a case for grouping these optional connectivities together by their location/source element. e.g.
edge:
edge_node_connectivity
(optionally)
edge_face_connectivity
face:
face_node_connectivity
(optionally)
face_edge_connectivity
face_face_connectivity
with boundary_node_connectivity as a special case on its own.
In this way, connectivities would be grouped with other connectivities which share their dimensions.
(As an aside, I don't believe we currently keep hold of the NetCDF dimension associated with boundary_node_connectivity, I'm not sure if this is something we should be concerned about. I had a look at what UGRID says about them and it seems to suggest that boundary_node_connectivity can have ancillary variables attached which I don't know if we support. This is probably not core functionality, but it might be worth recording as an issue for later.)
With that said, I could also imagine an argument that something like edge_face_connectivity is just as much assiciated with faces as it is edges. I'm not necessarily asking for a change, but I thought it would be worth at least raising the point for discussion.
There was a problem hiding this comment.
At present I am purposely listing the non-optional 'edge_node' and 'face_node' connectivities in their "element section", as you suggest.
In my view, the "essential" connectivities really have a different scope to the 'optional' ones, and should be treated differently.
I think If we put the 'optional' ones into their relevant sections, that's maybe not so clear.
In that case I think I would to list the required information first, and add the optional connectivities after the coords (if any). Whether they need to be labelled 'optional' I'm not quite so sure.
As examples from an actual file ...
CURRENT
Mesh : 'Topology data of 2D unstructured mesh'
topology_dimension: 2
node
node_dimension: 'nMesh2d_half_levels_node'
node coordinates
<AuxCoord: longitude / (degrees) <lazy> shape(55298,)>
<AuxCoord: latitude / (degrees) <lazy> shape(55298,)>
edge
edge_dimension: 'nMesh2d_half_levels_edge'
edge_node_connectivity: <Connectivity: Maps every edge/link to two nodes that it connects. / (unknown) <lazy> shape(110592, 2)>
edge coordinates
<AuxCoord: longitude / (degrees) <lazy> shape(110592,)>
<AuxCoord: latitude / (degrees) <lazy> shape(110592,)>
face
face_dimension: 'nMesh2d_half_levels_face'
face_node_connectivity: <Connectivity: Maps every face to its corner nodes. / (unknown) <lazy> shape(55296, 4)>
face coordinates
<AuxCoord: longitude / (degrees) <lazy> shape(55296,)>
<AuxCoord: latitude / (degrees) <lazy> shape(55296,)>
optional connectivities
face_face_connectivity: <Connectivity: Indicates which other faces neighbor each face / (no_unit) <lazy> shape(55296, 4)>
face_edge_connectivity: <Connectivity: Maps every face to its edges. / (unknown) <lazy> shape(55296, 4)>
edge_face_connectivity: <Connectivity: neighbor faces for edges / (unknown) <lazy> shape(110592, 2)>
long_name: 'Topology data of 2D unstructured mesh'
var_name: 'Mesh2d_half_levels'
OPTIONALS IN OWN SECTIONS
Mesh : 'Topology data of 2D unstructured mesh'
topology_dimension: 2
node
node_dimension: 'nMesh2d_half_levels_node'
node coordinates
<AuxCoord: longitude / (degrees) <lazy> shape(55298,)>
<AuxCoord: latitude / (degrees) <lazy> shape(55298,)>
edge
edge_dimension: 'nMesh2d_half_levels_edge'
edge_node_connectivity: <Connectivity: Maps every edge/link to two nodes that it connects. / (unknown) <lazy> shape(110592, 2)>
edge coordinates
<AuxCoord: longitude / (degrees) <lazy> shape(110592,)>
<AuxCoord: latitude / (degrees) <lazy> shape(110592,)>
edge_face_connectivity: <Connectivity: neighbor faces for edges / (unknown) <lazy> shape(110592, 2)>
face
face_dimension: 'nMesh2d_half_levels_face'
face_node_connectivity: <Connectivity: Maps every face to its corner nodes. / (unknown) <lazy> shape(55296, 4)>
face coordinates
<AuxCoord: longitude / (degrees) <lazy> shape(55296,)>
<AuxCoord: latitude / (degrees) <lazy> shape(55296,)>
face_face_connectivity: <Connectivity: Indicates which other faces neighbor each face / (no_unit) <lazy> shape(55296, 4)>
face_edge_connectivity: <Connectivity: Maps every face to its edges. / (unknown) <lazy> shape(55296, 4)>
long_name: 'Topology data of 2D unstructured mesh'
var_name: 'Mesh2d_half_levels'
OPTIONALS LABELLED AS SUCH
Mesh : 'Topology data of 2D unstructured mesh'
topology_dimension: 2
node
node_dimension: 'nMesh2d_half_levels_node'
node coordinates
<AuxCoord: longitude / (degrees) <lazy> shape(55298,)>
<AuxCoord: latitude / (degrees) <lazy> shape(55298,)>
edge
edge_dimension: 'nMesh2d_half_levels_edge'
edge_node_connectivity: <Connectivity: Maps every edge/link to two nodes that it connects. / (unknown) <lazy> shape(110592, 2)>
edge coordinates
<AuxCoord: longitude / (degrees) <lazy> shape(110592,)>
<AuxCoord: latitude / (degrees) <lazy> shape(110592,)>
optional connectivities
edge_face_connectivity: <Connectivity: neighbor faces for edges / (unknown) <lazy> shape(110592, 2)>
face
face_dimension: 'nMesh2d_half_levels_face'
face_node_connectivity: <Connectivity: Maps every face to its corner nodes. / (unknown) <lazy> shape(55296, 4)>
face coordinates
<AuxCoord: longitude / (degrees) <lazy> shape(55296,)>
<AuxCoord: latitude / (degrees) <lazy> shape(55296,)>
optional connectivities
face_face_connectivity: <Connectivity: Indicates which other faces neighbor each face / (no_unit) <lazy> shape(55296, 4)>
face_edge_connectivity: <Connectivity: Maps every face to its edges. / (unknown) <lazy> shape(55296, 4)>
long_name: 'Topology data of 2D unstructured mesh'
var_name: 'Mesh2d_half_levels'
WITH BOUNDARIES
Mesh : 'Topology data of 2D unstructured mesh'
topology_dimension: 2
node
node_dimension: 'nMesh2d_half_levels_node'
node coordinates
<AuxCoord: longitude / (degrees) <lazy> shape(55298,)>
<AuxCoord: latitude / (degrees) <lazy> shape(55298,)>
edge
edge_dimension: 'nMesh2d_half_levels_edge'
edge_node_connectivity: <Connectivity: Maps every edge/link to two nodes that it connects. / (unknown) <lazy> shape(110592, 2)>
edge coordinates
<AuxCoord: longitude / (degrees) <lazy> shape(110592,)>
<AuxCoord: latitude / (degrees) <lazy> shape(110592,)>
optional connectivities:
edge_face_connectivity: <Connectivity: neighbor faces for edges / (unknown) <lazy> shape(110592, 2)>
face
face_dimension: 'nMesh2d_half_levels_face'
face_node_connectivity: <Connectivity: Maps every face to its corner nodes. / (unknown) <lazy> shape(55296, 4)>
face coordinates
<AuxCoord: longitude / (degrees) <lazy> shape(55296,)>
<AuxCoord: latitude / (degrees) <lazy> shape(55296,)>
optional connectivities:
face_face_connectivity: <Connectivity: Indicates which other faces neighbor each face / (no_unit) <lazy> shape(55296, 4)>
face_edge_connectivity: <Connectivity: Maps every face to its edges. / (unknown) <lazy> shape(55296, 4)>
boundary
boundary_dimension: 'nMesh2d_half_levels_boundary'
boundary_node_connectivity: <Connectivity: unknown / (unknown) <lazy> shape(1417, 2)>
long_name: 'Topology data of 2D unstructured mesh'
var_name: 'Mesh2d_half_levels'
There was a problem hiding this comment.
(After all that wordage)
I conclude that this change its perfectly possible.
If doing this, I think it makes sense to put boundaries in their own section, and report the 'boundary_dimension', like this. But in that case I think we should support it in the Mesh object too.
But I think there are some CONs too :
- the existing output is more concise
- the existing output is possibly slightly clearer
- I'm not convinced that we really know what the "status" of boundaries is : it seems to be a feature that is not fully developed + it's not clear quite how it is intended to be used or what for
There was a problem hiding this comment.
I agree that this is definitely a worthwhile thing to discuss. The hard part is knowing what the best option is! I'm strugglling to come to a conclusion on this. I'm not terribly familiar with the area myself so finding it hard to decide what a user would expect. However, I am leaning towards keeping things as they are , primarily because I like the conciseness of the original output, also @stephenworsley's point:
With that said, I could also imagine an argument that something like edge_face_connectivity is just as much assiciated with faces as it is edges.
This is an interesting point!
I think it might be simpler to keep the optional connectivities in their own section for now. But readdress this point at a later stage. Would you be okay with that @stephenworsley ?
There was a problem hiding this comment.
Have checked with @stephenworsley offline. I will capture this point in an issue and we can address it (and come to a decision!) as a follow up activity. But I would like to merge this in as we have it
There was a problem hiding this comment.
Yeah, I think either way is workable and keeping it as it is currently is probably for the best for the moment. I imagine this could be something we could return to when we have a better idea of what to do with the boundaries.
Closes #4488, #4494
Linked to #4501
This is just an initial draft version, for discussing the new behaviour + printed output format
Please review for behaviours (output results), and not the code (yet!)
Some general notes :
Cube.__str__output__repr__produces a minimal one-line form, and the__str__a detailed multiline printout, like the Cube ones