Ucubes summaries#27
Conversation
|
|
||
| """ | ||
| name = super()._summary_dim_name(dim) | ||
| if self.ugrid and dim == self.ugrid.cube_dim: |
There was a problem hiding this comment.
| if self.ugrid and dim == self.ugrid.cube_dim: | |
| if self.ugrid is not None and dim == self.ugrid.cube_dim: |
The original syntax is nice and efficient, but very easy to misinterpret. Is my suggestion equivalent? And I assume that dim == self.ugrid.cube_dim definitely doesn't trip over if self.ugrid doesn't exist?
| ][0] | ||
|
|
||
| # Indent the mesh details 4 spaces more than that. | ||
| indent = " " * (indent + 4) |
There was a problem hiding this comment.
| indent = " " * (indent + 4) | |
| indent += (" " * 4) |
I don't think the original is written the correct way round. Wouldn't it multiply an existing indent rather than just adding to it?
There was a problem hiding this comment.
I was wrong: I hadn't understood that indent is actually numerical at this stage. It only becomes text on L86.
My new suggestion is to give the precursor a different name rather than indent, to avoid confusion.
| Constructs a :class:`CubeUgrid` referencing the appropriate file mesh, | ||
| and makes a new `UCube` of which this is the '.ugrid' property. |
There was a problem hiding this comment.
Need to say somewhere that this actually returns a UCube.
| # Return a new UCube, based on the provided Cube, and replacing it | ||
| # in the caller (and as returned to user). |
There was a problem hiding this comment.
I don't follow. To me, it doesn't look like anything is replaced anymore - this function no longer modifies cube, instead it returns new_result_cube.
There was a problem hiding this comment.
Have attempted to rephrase in a more explicit way. Believe @pp-mo was referring to the downstream change he introduced to iris.fileformats.netcdf.load_cubes()
|
Hi @lbdreyer, please could you see if you think I've addressed my own comments, and that this PR looks OK to you? Thanks! |
| The cube to be post-processed | ||
|
|
||
| Returns: | ||
| :class:`iris_ugrid.ucube.UCube` |
There was a problem hiding this comment.
... or None.
It may be worth mentioning that?
| from iris import Constraint | ||
| from iris.tests import IrisTest, get_data_path | ||
|
|
||
| from iris.cube import CubeList |
There was a problem hiding this comment.
I don't understand this order of these imports.
Why isn't it:
| from iris import Constraint | |
| from iris.tests import IrisTest, get_data_path | |
| from iris.cube import CubeList | |
| from iris import Constraint | |
| from iris.cube import CubeList | |
| from iris.tests import IrisTest, get_data_path |
There was a problem hiding this comment.
I'm just going to remove the from iris.tests import altogether, since we're already importing iris.tests on L9.
|
This looks good to me now. Once travis is finished I will merge in @trexfeathers I think merge is blocked until you approve it currently says "Merging can be performed automatically once the requested changes are addressed." |
4 participants
Re-spin of #26, as I had some problems with stickler testing
Make unstructured cubes load as
UCubes (i.e. a specialised subclass).And then to make them print augmented cube summaries.
Along with SciTools/iris#3914, this effectively completes the import of SciTools/iris#3688 ,which was the POC code for this in the irtis "ng-vat" branch.
Unfinished business:
UCube._repr_html_ideally, should test that loading can (correctly) produce a mixture of UCube-s and Cube-s.( second thoughts : not very important. not going to bother )