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

Enable lazy computation of wind vector rotation #4972

Merged
merged 9 commits into from Mar 31, 2023

Conversation

tinyendian
Copy link
Contributor

@tinyendian tinyendian commented Sep 18, 2022

🚀 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:

  • Coordinate data is not lazy at this point - I can have a look at lazy evaluation if you think it would be worthwhile, e.g., if the same routine should also handle unstructured datasets with 2D coordinate fields
  • An 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.
  • I added a new test to 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

@tinyendian tinyendian changed the title Lazy rotate winds issue 4934 Enable lazy computation of wind vector rotation Sep 19, 2022
@codecov
Copy link

codecov bot commented Mar 31, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (b8f5eea) 89.27% compared to head (5ea2a34) 89.28%.

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           
Impacted Files Coverage Δ
lib/iris/analysis/cartography.py 86.41% <100.00%> (+0.37%) ⬆️

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

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.

@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 👍

@bjlittle bjlittle merged commit ba3ac6d into SciTools:main Mar 31, 2023
15 checks passed
@bjlittle bjlittle mentioned this pull request Mar 31, 2023
@tinyendian tinyendian deleted the lazy-rotate-winds-issue-4934 branch April 2, 2023 20:51
HGWright added a commit to HGWright/iris that referenced this pull request Apr 3, 2023
* '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)
tkknight added a commit to tkknight/iris that referenced this pull request Apr 4, 2023
* 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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants