-
Notifications
You must be signed in to change notification settings - Fork 281
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
Enable lazy computation of wind vector rotation #4972
Enable lazy computation of wind vector rotation #4972
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #4972 /- ##
=======================================
Coverage 89.27% 89.28%
=======================================
Files 88 88
Lines 22258 22269 11
Branches 4867 4870 3
=======================================
Hits 19871 19882 11
Misses 1641 1641
Partials 746 746
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tinyendian Awesome contribution! 💯
Apologies for the super tardy response.
I'll follow-up with PR raising the minimum version of dask
, given the usage of dask.array.ma.empty_like
👍
* 'main' of github.com:SciTools/iris: Bump scitools/workflows from 2023.03.2 to 2023.03.3 (SciTools#5227) raise dask min pin (SciTools#5225) Enable lazy computation of wind vector rotation (SciTools#4972)
* upstream/main: (274 commits) Cf cell method (SciTools#5224) Bump scitools/workflows from 2023.03.3 to 2023.04.1 (SciTools#5231) [pre-commit.ci] pre-commit autoupdate (SciTools#5230) Bump scitools/workflows from 2023.03.2 to 2023.03.3 (SciTools#5227) raise dask min pin (SciTools#5225) Enable lazy computation of wind vector rotation (SciTools#4972) Benchmark runner script (SciTools#5215) add locks readme (SciTools#5222) use explicit version for ci refresh-lockfiles gha (SciTools#5221) SciTools#5180 do not run publish-to-test-pypi on forks (SciTools#5220) Fix Makefile for consequences of SciTools#5204. (SciTools#5217) Bump actions/stale from 7 to 8 (SciTools#5208) Remove Iris' TestRunner (SciTools#5205) standardize requirements structure (SciTools#5204) Updated environment lockfiles (SciTools#5199) Add histogram convenience for passing Iris objects to plt.hist (SciTools#5189) Updated environment lockfiles (SciTools#5192) announce new contributor in whatsnew (SciTools#5198) iris.util.new_axis anonymous new dimension fix (SciTools#5194) Lockfiles and pydata-sphinx-theme fix (SciTools#5188) ...
🚀 Pull Request
Description
This pull request enables lazy wind rotation, where the pevious implementation triggered data loads in the wind component cubes. Wind rotation can thus be computed out-of-core via Dask. If only one of the two wind component cubes has lazy data, the other cube will be converted to lazy data, following the default behaviour of Dask arrays, and avoiding data loads that a user might not expect. The pull request tries to minimise changes to the code.
A few more comments:
empty_like
masked array creation function was only very recently added to Dask, I thus chose the following workaround to avoid having to upgrade Dask version,ut_cube = ut_cube.copy( data=da.ma.masked_array(ut_cube.core_data()) )
In my understanding, the new cube instance of
ut_cube
should receive all metadata from the old version, and the empty Dask array that is held by the old cube version changes ownership to the new one via reference passing while receiving its mask "upgrade" - I hope that such change of ownership is safe, I can otherwise get Dask to create a new data array.test_rotate_winds.py
which checks if "laziness" is indeed preserved, if results are consistent with non-lazy computation, and if the function modifies the input wind component cubes (as a safeguard against data handling mistakes)closes #4934
Consult Iris pull request check list