Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add summary of ugrid to cube print out #3688

Merged
merged 5 commits into from
Mar 24, 2020

Conversation

lbdreyer
Copy link
Member

@lbdreyer lbdreyer commented Mar 18, 2020

DO NOT MERGE: Blocked by #3684

NOW READY FOR REVIEW

This extends the cube repr to include ugrid information.

  • In the cube header, the dimension that is mapped by the unstructured mesh is identified with a preceding *.
  • A new section (ugrid information) is added.

The repr looks like:

air_potential_temperature / (K)     (-- : 1; full_levels: 39; *-- : 864)
     Dimension coordinates:
          full_levels                   -               x         -
     Auxiliary coordinates:
          time                          x               -         -
     ugrid information:
          Mesh2d_full_levels.face       -               -         x
          topology_dimension: 2
          node_coordinates: longitude latitude

And the repr htm looks like
image

Important things to note:

  • Previously we had discussed including a vertices per face: min 4 max 4. I decided against this as to compute it, we need to load the data from the face_node_connectivity variable (if it exists) and considering this may be big I thought it wasn't worth the performance cost
  • I haven't added tests (other than extending a test added in Ugrid cf load #3684). Our testing of cube print summaries has always been pretty limited and I thought that since we are working with PoC code it may not be necessary?? But happy to add some test if you think that would be best

@pp-mo
Copy link
Member

pp-mo commented Mar 18, 2020

Hi @lbdreyer this should benefit from rebase, since #3685

@bjlittle bjlittle added the PoC label Mar 19, 2020
@bjlittle bjlittle modified the milestone: PoC Mar 19, 2020
@lbdreyer lbdreyer changed the title WIP: Add summary of ugrid to cube print out Add summary of ugrid to cube print out Mar 20, 2020
@trexfeathers trexfeathers self-assigned this Mar 23, 2020
" "
):
node_var = self.dataset.variables[node_var_name]
node_coordinates.append(node_var.getncattr("standard_name"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't help feeling we should be applying the same here as in Iris --> standard_name or long_name or var_name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was a bit unsure about this bit. I'll do the different naming as you suggest.

Copy link
Contributor

@trexfeathers trexfeathers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK, works fine on a number of files I tried, thanks @lbdreyer! Please could you address / push back on a few code layout things I've highlighted?

@pp-mo
Copy link
Member

pp-mo commented Mar 23, 2020

Just taking a quick look.
If you can manage it, I do think a simple testcase example would be useful -- probably a quick integration test with the existing file.
How about : lbdreyer#8
?

@pp-mo
Copy link
Member

pp-mo commented Mar 23, 2020

Just taking a quick look.

Otherwise, only skimmed but LGTM : the additions to ugrid_cf_reader seem fine.

@lbdreyer
Copy link
Member Author

Thanks @trexfeathers and @pp-mo for the review! I have dealt with your comments in the most recent commit.
Please re-review

@trexfeathers
Copy link
Contributor

Thanks for addressing @lbdreyer, love the improvement to vector_summary 💖.
Won't merge this myself since it's probably best that @pp-mo checks the names change, given he originally caught that and I didn't.

@pp-mo
Copy link
Member

pp-mo commented Mar 24, 2020

TBH this code area is a bit nasty, and I really don't envy you doing this, so 🥇 for taking it on @lbdreyer !!
As I've no special understanding here, I feel I could easily still be missing something, but it clearly functions, so I think we should bank it + deal with further problems if+when they arise.

@pp-mo pp-mo merged commit 69ea060 into SciTools:ng-vat Mar 24, 2020
pp-mo added a commit to pp-mo/iris that referenced this pull request Oct 24, 2020
@pp-mo pp-mo mentioned this pull request Oct 26, 2020
pp-mo added a commit to pp-mo/iris that referenced this pull request Oct 27, 2020
trexfeathers pushed a commit that referenced this pull request Oct 30, 2020
* Factor out cube.summary dim-naming + remove code for (nonexistent) multi-dim dim-coords.

* Import cube.summary rework from #3688, but omitting ucube-specific parts.

* Rename 'vector_summary' result variable of same-named function.

* Use generic DimensionalMetadata.cube_dims in place of 'dim_function' arg.

* Replace boilerplate calls to 'vector_summary' with generic code controlled by a table.

* Define cube summary 'vector sections' via a function, which iris-ugrid can override.

* Vector section summary titles used by cube.repr_html() are drawn from the cube itself.

* Test for Cube._summary_dim_name().

* Tests for Cube._summary_vector_sections_info().

* Tiny docstring fixes.

* Review changes.

* Simpler calculation of vector elements, and handling of scalar elements.

* Test fix.
@lbdreyer lbdreyer deleted the print_cube_ugrid branch June 23, 2021 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants