Skip to content

Implement lazy area weights #5658

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
Merged
trexfeathers merged 12 commits into from
Apr 8, 2024
Merged

Implement lazy area weights #5658

trexfeathers merged 12 commits into from
Apr 8, 2024

Conversation

@schlunma
Copy link
Contributor

@schlunma schlunma commented Dec 19, 2023 •
edited
Loading

🚀 Pull Request

Description

This PR makes iris.analysis.cartography.area_weights lazy by providing the keyword arguments compute and chunks:

def area_weights(cube, normalize=False, compute=True, chunks=None):

These defaults ensure full backwards-compatibility. chunks=None results in using the cube data's chunks for the weights array, which is a resonable default since these weights will probably be used with the cube's data anyway.

This requires #5620, so still in draft mode. Apart from that, this PR is ready.

Closes #5611

Consult Iris pull request check list

@schlunma schlunma changed the title Lazy area weights Implement lazy area weights Dec 19, 2023
@codecov
Copy link

codecov bot commented Dec 19, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.77%. Comparing base (7c313ff) to head (e6985e3).
Report is 4 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #5658   +/-   ##
=======================================
  Coverage   89.76%   89.77%           
=======================================
  Files          93       93           
  Lines       22982    22995   +13     
  Branches     5006     5011    +5     
=======================================
+ Hits        20630    20643   +13     
  Misses       1622     1622           
  Partials      730      730

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@trexfeathers
Copy link
Contributor

trexfeathers commented Dec 20, 2023

Thanks for the hard work @schlunma. #5620 will be reviewed by @pp-mo in the new year, so #5658 can get moving again once that is sorted.

@schlunma schlunma mentioned this pull request Feb 28, 2024
Merged
10 tasks
@trexfeathers trexfeathers self-assigned this Feb 29, 2024
@trexfeathers trexfeathers marked this pull request as ready for review April 5, 2024 11:43
@CLAassistant
Copy link

CLAassistant commented Apr 5, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

trexfeathers
trexfeathers requested changes Apr 5, 2024
View reviewed changes
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.

This looks great, thanks @schlunma! Just a tiny request please

@schlunma
Copy link
Contributor Author

schlunma commented Apr 5, 2024

Just had a second look on this. I think it's actually better to use cube.lazy_data().chunks instead of cube.lazy_data().chunksize to make sure that the chunk structure is exactly identical in the weights and the original data. It probably won't matter, but it feels better to use .chunks here...

@schlunma schlunma marked this pull request as draft April 6, 2024 07:09
@schlunma
Copy link
Contributor Author

schlunma commented Apr 6, 2024

After having a third look on it, I think it's better to leave full control of the chunks to the user (i.e., if chunks=None, also use chunks=None in the broadcast_to_shape call instead of the cube's chunks). Otherwise, there is no way a user can specify to use "no special chunking", which might be confusing.

Sorry for all the back and forth, with this I am happy with the PR!

@schlunma schlunma marked this pull request as ready for review April 6, 2024 08:03
trexfeathers
trexfeathers approved these changes Apr 8, 2024
View reviewed changes
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.

OK these changes make equal sense to me

@trexfeathers trexfeathers merged commit 600c47c into SciTools:main Apr 8, 2024
tkknight added a commit to tkknight/iris that referenced this pull request Apr 10, 2024
@tkknight
Merge remote-tracking branch 'upstream/main' into replace_napolean_wi…
aa4e24b 
…th_numpydoc

* upstream/main: (39 commits)
  Bump scitools/workflows from 2024.03.3 to 2024.04.0 (SciTools#5907)
  [pre-commit.ci] pre-commit autoupdate (SciTools#5906)
  Updated environment lockfiles (SciTools#5904)
  Ignore flaticon.com in linkchecks. (SciTools#5905)
  Implement lazy area weights (SciTools#5658)
  Add option to specify chunks in `iris.util.broadcast_to_shape` (SciTools#5620)
  Unpin sphinx (SciTools#5901)
  DOC: clarify save_pairs_from_cube docstring (SciTools#5783)
  Restore latest Whats New files.
  Whats new updates for `v3.9.0rc0` (SciTools#5899)
  nep29: drop py39 and support py312 (SciTools#5894)
  Support NetCDF v3 files in chunking control code. (SciTools#5897)
  Avoid computing lazy scalar coordinates when printing a Cube (v2) (SciTools#5896)
  Force pytest colour output on GitHub Actions (SciTools#5895)
  Make typing 3.9 compatible.
  Improve typing readability.
  Updated environment lockfiles (SciTools#5892)
  [pre-commit.ci] pre-commit autoupdate
  What's New entry for SciTools#5740 .
  Iris to GeoVista conversion (SciTools#5740)
  ...
@schlunma schlunma deleted the lazy_area_weights branch April 30, 2024 13:30
@schlunma schlunma mentioned this pull request Jan 16, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@trexfeathers trexfeathers trexfeathers approved these changes

Assignees

@trexfeathers trexfeathers

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Lazy iris.analysis.cartography.area_weights

5 participants

@schlunma @trexfeathers @CLAassistant @pp-mo @bouweandela