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.

Sign up for GitHub

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

Jump to bottom

Mesh Loading (AVD-1813) #4262

Merged
merged 26 commits into from
Aug 23, 2021
Merged

Mesh Loading (AVD-1813) #4262

merged 26 commits into from
Aug 23, 2021

Conversation

trexfeathers
Copy link
Contributor

@trexfeathers trexfeathers commented Jul 29, 2021 •
edited
Loading

🚀 Pull Request

Description

Load Meshes without associated Cubes.

TODO:

  • Integration tests
  • [ ] Performance tests

Consult Iris pull request check list

@trexfeathers trexfeathers requested a review from pp-mo July 29, 2021 09:22
pp-mo
pp-mo requested changes Jul 30, 2021
View reviewed changes
Copy link
Member

@pp-mo pp-mo left a comment

Choose a reason for hiding this comment

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

Some changes suggested, but no show-stoppers. Feel free to mark things as 'resolved' in my absence.
I've only looked properly at the implementation code : I haven't time now to scan all the tests for correct coverage + someone else will have to take this on
But it's looking good to me !

lib/iris/experimental/ugrid/__init__.py Show resolved Hide resolved
lib/iris/fileformats/netcdf.py Show resolved Hide resolved
lib/iris/experimental/ugrid/__init__.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/__init__.py Outdated Show resolved Hide resolved
@trexfeathers
Copy link
Contributor Author

This branch doesn't yet support performance testing, so that will be run locally for now.

@trexfeathers trexfeathers marked this pull request as ready for review July 30, 2021 17:27
pp-mo
pp-mo reviewed Aug 19, 2021
View reviewed changes
Copy link
Member

@pp-mo pp-mo left a comment

Choose a reason for hiding this comment

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

Basically all quite nice, but I raised a few questions.
So here's some things to think about, anyway.
Unfortunately I'm still not really quite done -- I need to think about the coverage of the main new tests a bit more.

lib/iris/experimental/ugrid/__init__.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/__init__.py Outdated Show resolved Hide resolved
lib/iris/tests/integration/experimental/test_ugrid_load.py Show resolved Hide resolved
lib/iris/tests/unit/experimental/ugrid/test_CFUGridMeshVariable.py Show resolved Hide resolved
lib/iris/tests/unit/experimental/ugrid/test_load_mesh.py Outdated Show resolved Hide resolved
lib/iris/tests/unit/experimental/ugrid/test_load_meshes.py Show resolved Hide resolved
lib/iris/tests/unit/experimental/ugrid/test_load_meshes.py Outdated Show resolved Hide resolved
lib/iris/tests/unit/experimental/ugrid/test_load_meshes.py Outdated Show resolved Hide resolved
pp-mo
pp-mo reviewed Aug 20, 2021
View reviewed changes
Copy link
Member

@pp-mo pp-mo left a comment

Choose a reason for hiding this comment

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

Ok, that's my part#2
and I think that's now all
except that I'm still thinking about the duplication of the CFUGridMeshVariable creation I mentioned before : I'm still thinking about whether I think that is something different from the existing cases, or not. But that shouldn't take me much longer, and I'll let you know shortly..

lib/iris/tests/unit/experimental/ugrid/test_load_meshes.py Show resolved Hide resolved
lib/iris/tests/unit/experimental/ugrid/test_load_meshes.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/__init__.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/__init__.py Show resolved Hide resolved
lib/iris/experimental/ugrid/__init__.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/__init__.py Outdated Show resolved Hide resolved
@pp-mo
Copy link
Member

pp-mo commented Aug 20, 2021 •
edited
Loading

It looks like we need to make Meshes hashable (should fix failing tests).
See : trexfeathers#5

Logical arguments against, exactly as for Cube, but I think it's forgiveable.

#4290 will tell us if this fixes the remaining problems...
If so, and you accept the fix @trexfeathers , then I'm good to merge this

@pp-mo pp-mo mentioned this pull request Aug 23, 2021
Closed
@pp-mo pp-mo merged commit cd420bb into SciTools:mesh-data-model Aug 23, 2021
@trexfeathers
Copy link
Contributor Author

Thanks @pp-mo!

@trexfeathers trexfeathers deleted the AVD-1813_mesh_loading branch August 31, 2021 15:44
tkknight added a commit to tkknight/iris that referenced this pull request Sep 19, 2021
@tkknight
Merge branch 'main' into theme_play
fc570d3 
* main: (71 commits)
  Skip TestConstrainedLoad if data missing (SciTools#4319)
  Add 'Good First Issue' label to reasons an issue doesn't go stale (SciTools#4317)
  Gallery: simplify quiver example (SciTools#4120)
  Improve styling in a minor way in docs (SciTools#4314)
  bump version (SciTools#4310)
  Made clear we only test on Linux. (SciTools#4309)
  Updated environment lockfiles (SciTools#4308)
  Include Discussions in Getting Involved. (SciTools#4307)
  Fixed text to show as link. (SciTools#4305)
  [pre-commit.ci] pre-commit autoupdate (SciTools#4299)
  Updated environment lockfiles (SciTools#4298)
  cartopy feature download (SciTools#4304)
  Mesh Loading (AVD-1813) (SciTools#4262)
  reset whatsnew latest (SciTools#4288)
  Updated environment lockfiles (SciTools#4289)
  Update cube.py (SciTools#4287)
  Integrated whatsnew for v3.1 release (rc0) (SciTools#4285)
  Version changes and final whatsnew tweaks for 3v1rc0. (SciTools#4284)
  Missing whatsnew entries for 3.1 release. (SciTools#4283)
  Update CF standard name table to v77 (SciTools#4282)
  ...
tkknight added a commit to tkknight/iris that referenced this pull request Sep 22, 2021
@tkknight
Merge branch 'main' into votable_issues
a0c947b 
* main: (94 commits)
  added support for make html-noapi and html-quick for the docs build (SciTools#4333)
  Refactor some netcdf save code (SciTools#4301)
  [pre-commit.ci] pre-commit autoupdate (SciTools#4329)
  Update to loading docs to cover absence of 'or' for constraints (SciTools#4321)
  update latest.rst.template (SciTools#4323)
  stable cartopy feature download (SciTools#4328)
  Skip TestConstrainedLoad if data missing (SciTools#4319)
  Add 'Good First Issue' label to reasons an issue doesn't go stale (SciTools#4317)
  Gallery: simplify quiver example (SciTools#4120)
  Improve styling in a minor way in docs (SciTools#4314)
  bump version (SciTools#4310)
  Made clear we only test on Linux. (SciTools#4309)
  Updated environment lockfiles (SciTools#4308)
  Include Discussions in Getting Involved. (SciTools#4307)
  Fixed text to show as link. (SciTools#4305)
  [pre-commit.ci] pre-commit autoupdate (SciTools#4299)
  Updated environment lockfiles (SciTools#4298)
  cartopy feature download (SciTools#4304)
  Mesh Loading (AVD-1813) (SciTools#4262)
  reset whatsnew latest (SciTools#4288)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pp-mo pp-mo pp-mo approved these changes

Assignees
No one assigned
Labels
Feature: UGRID
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@trexfeathers @pp-mo