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

Mesh recombine #4437

Merged
merged 7 commits into from
Nov 29, 2021
Merged

Mesh recombine #4437

merged 7 commits into from
Nov 29, 2021

Conversation

pp-mo
Copy link
Member

@pp-mo pp-mo commented Nov 25, 2021

First-version for "recombining" sub-mesh information.

It's worth noting that supporting location-index-sets would make this easier, as then the sub-region cubes would simply be referred to a common mesh+location, and the awkward 'special index coords' would not be required.

Without that, though, various alternative APIs are feasible + I could still be persuaded to change.
In particular, we could replace 'full_mesh_cube' with 'mesh' + 'location' args -- it's effectively more logical, as we can deduce everything else from the region cubes.
So, that might be slightly neater, but more work is required . Where elements could map both mesh and non-mesh dims, it could get complicated.

TODOs:

  • tests for all the consistency checks
  • tests for other features + functions
    • transposed forms
    • zero + multiple non-mesh (structured) dimensions
    • dtypes
    • mask behaviours
    • NaNs
  • tests for lazy behaviour
  • benchmarks showing sensible use of memory in lazy evaluation (as this is a known practical requirement)

@pp-mo pp-mo requested a review from bjlittle November 25, 2021 14:21
@pp-mo pp-mo marked this pull request as draft November 25, 2021 14:36
Copy link
Member

@bjlittle bjlittle left a comment

Choose a reason for hiding this comment

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

@pp-mo Looks good... still need to go through the tests, but I can sweep through them later when they're all there 👍

lib/iris/experimental/ugrid/utils.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/utils.py Show resolved Hide resolved
lib/iris/experimental/ugrid/utils.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/utils.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/utils.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/utils.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/utils.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/utils.py Show resolved Hide resolved
lib/iris/experimental/ugrid/utils.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/utils.py Show resolved Hide resolved
@pp-mo
Copy link
Member Author

pp-mo commented Nov 29, 2021

A point that just occurred to me..
I have realised that the indices could be checked against the original mesh ~fairly cheaply.
I.E. as everything has coords, we could test that each submesh cube has
all(submesh.coord(xyname).points == meshcube.coord(xyname)[submesh.indices].points)

It would be sort-of logical to do this, to test the user contract that the submesh-s 'belong' to the original mesh.
But I'm thinking we don't have much enthusiasm for this, even though the runtime cost is probably not very high ??

@pp-mo
Copy link
Member Author

pp-mo commented Nov 29, 2021

Ok, some rather noisy changes there.
I think I covered everything.
Please re-review @bjlittle

@pp-mo
Copy link
Member Author

pp-mo commented Nov 29, 2021

Update : added tests for various calculation behaviours.
TODO: tests for input error checking (now done)

@pp-mo
Copy link
Member Author

pp-mo commented Nov 29, 2021

Update: added the rest of the tests.

I think this is now GTG

@pp-mo pp-mo marked this pull request as ready for review November 29, 2021 17:52
Copy link
Member

@bjlittle bjlittle left a comment

Choose a reason for hiding this comment

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

@pp-mo This is a lovely contribution, awesome thanks 🍻

@bjlittle bjlittle merged commit 7bb929b into SciTools:main Nov 29, 2021
@pp-mo pp-mo deleted the mesh_recombine branch December 3, 2021 20:05
tkknight added a commit to tkknight/iris that referenced this pull request Dec 7, 2021
* main:
  [pre-commit.ci] pre-commit autoupdate (SciTools#4452)
  Whatsnew (SciTools#4451)
  Explicitly define tests so nose can find them (SciTools#4450)
  Updated environment lockfiles (SciTools#4449)
  Update CI environment lockfiles (SciTools#4424)
  Disable GHA workflows on non-SciTools branches (SciTools#4444)
  Add new contributor to whatsnew (SciTools#4443)
  Use dask-core instead of dask in ci (SciTools#4434)
  Mesh recombine (SciTools#4437)
  Mesh full comparison (SciTools#4439)
  Only try to work out the differences between points for multiple (SciTools#4367)
  [pre-commit.ci] pre-commit autoupdate (SciTools#4430)
  Fix license PyPI classifier (SciTools#4435)
  Whatsnew for PR SciTools#4367 (SciTools#4440)
tkknight added a commit to tkknight/iris that referenced this pull request Dec 14, 2021
* upstream/main: (78 commits)
  Updated environment lockfiles (SciTools#4458)
  remove asv package dependency (SciTools#4456)
  cube.aggregated_by output bounds (SciTools#4315)
  [pre-commit.ci] pre-commit autoupdate (SciTools#4452)
  Whatsnew (SciTools#4451)
  Explicitly define tests so nose can find them (SciTools#4450)
  Updated environment lockfiles (SciTools#4449)
  Update CI environment lockfiles (SciTools#4424)
  Disable GHA workflows on non-SciTools branches (SciTools#4444)
  Add new contributor to whatsnew (SciTools#4443)
  Use dask-core instead of dask in ci (SciTools#4434)
  Mesh recombine (SciTools#4437)
  Mesh full comparison (SciTools#4439)
  Only try to work out the differences between points for multiple (SciTools#4367)
  [pre-commit.ci] pre-commit autoupdate (SciTools#4430)
  Fix license PyPI classifier (SciTools#4435)
  Whatsnew for PR SciTools#4367 (SciTools#4440)
  Suggest type hinting (SciTools#4390)
  area weight regrid test fixes (SciTools#4432)
  Update latest.rst (SciTools#4425)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants