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

Allow smarter weights (cubes, coordinates, cell measures, or ancillary variables) for aggregation #5084

Merged
merged 25 commits into from
Mar 10, 2023

Conversation

schlunma
Copy link
Contributor

@schlunma schlunma commented Nov 25, 2022

🚀 Pull Request

Description

This PR allows using cubes, coordinates, cell measures, or ancillary variables as weights.

Example:

cube.collapsed(['latitude', 'longitude'], iris.analysis.SUM, weights=area_cube)

or

cube.collapsed(['latitude', 'longitude'], iris.analysis.SUM, weights='cell area')

This automatically handles potential unit conversions (e.g., multiply by m2 for area-weighted sums).

Closes #5082.


Consult Iris pull request check list

Copy link
Member

@lbdreyer lbdreyer left a comment

Choose a reason for hiding this comment

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

Thanks for this @schlunma ! I can see how this will be a really useful addition!

I've added some thoughts in the comments below. The key point I wanted to raise, as I mention here, is that I think the Weights class would be more suitable as a private class

lib/iris/analysis/__init__.py Outdated Show resolved Hide resolved
lib/iris/analysis/__init__.py Outdated Show resolved Hide resolved
lib/iris/analysis/__init__.py Outdated Show resolved Hide resolved
lib/iris/tests/test_lazy_aggregate_by.py Show resolved Hide resolved
lib/iris/tests/test_lazy_aggregate_by.py Show resolved Hide resolved
lib/iris/tests/test_aggregate_by.py Outdated Show resolved Hide resolved
lib/iris/tests/test_analysis.py Outdated Show resolved Hide resolved
lib/iris/analysis/__init__.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 6, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 🎉

Comparison is base (897a7cb) 89.24% compared to head (ab0a20f) 89.26%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5084      +/-   ##
==========================================
+ Coverage   89.24%   89.26%   +0.02%     
==========================================
  Files          88       88              
  Lines       22191    22233      +42     
  Branches     4855     4863       +8     
==========================================
+ Hits        19805    19847      +42     
  Misses       1641     1641              
  Partials      745      745              
Impacted Files Coverage Δ
lib/iris/analysis/__init__.py 91.89% <100.00%> (+0.41%) ⬆️
lib/iris/cube.py 90.56% <100.00%> (+0.02%) ⬆️

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.

@schlunma
Copy link
Contributor Author

schlunma commented Mar 6, 2023

Thanks for reviewing @lbdreyer! I think I addressed all your comments, let me know if you need anything else. 👍

About the line in the __array_finalize__ method of Weights that is not covered by tests:

I copy-pasted this from https://numpy.org/doc/stable/user/basics.subclassing.html#slightly-more-realistic-example-attribute-added-to-existing-array. However, if I understand the description of the __array_finalize__ in the docstring here correctly, then obj can only be None if the __new__ method of the child class calls the __new__ method of the parent class (here: np.ndarray) directly. Since the __new__ method of the child class does not do that but only uses np.asarray(...).view(cls), I think obj can never be None here, so we could probably safely remove this line. My only question would then be why the example uses this line? Maybe I overlooked something?

@schlunma
Copy link
Contributor Author

schlunma commented Mar 6, 2023

The linkcheck reports that the Twitter link is broken

 community/index: line   28) broken    https://twitter.com/scitools_iris - 403 Client Error: Forbidden for url: https://twitter.com/scitools_iris

For me that link works. Not sure if this is related to this PR.

@rcomer
Copy link
Member

rcomer commented Mar 6, 2023

I just re-ran the link check and it’s OK now.

Copy link
Member

@lbdreyer lbdreyer left a comment

Choose a reason for hiding this comment

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

Thanks for all the changes @schlunma !
I just have a couple of small fixes but otherwise this is very close!

lib/iris/tests/test_analysis.py Show resolved Hide resolved
lib/iris/tests/unit/cube/test_Cube.py Outdated Show resolved Hide resolved
lib/iris/tests/unit/cube/test_Cube.py Outdated Show resolved Hide resolved
lib/iris/tests/test_lazy_aggregate_by.py Show resolved Hide resolved
@lbdreyer
Copy link
Member

About the line in the array_finalize method of Weights that is not covered by tests:

I copy-pasted this from https://numpy.org/doc/stable/user/basics.subclassing.html#slightly-more-realistic-example-attribute-added-to-existing-array. However, if I understand the description of the array_finalize in the docstring here correctly, then obj can only be None if the new method of the child class calls the new method of the parent class (here: np.ndarray) directly. Since the new method of the child class does not do that but only uses np.asarray(...).view(cls), I think obj can never be None here, so we could probably safely remove this line. My only question would then be why the example uses this line? Maybe I overlooked something?

I agree. The numpy docs imply that of the three ways that __array_finalize__ can be called,

  1. explicit constructor _Weights(),
  2. view casting np.zeros((3,)).view(_Weights),
  3. new from template existing_weights_object[:1]

Only the explicit constructor may have obj as None. But as you have pointed out obj is always returned by the __new__ method as np.asarray(...).view(cls),. It is probably only relevant if you were to call super().__new__()
So I'd be happy were you to remove if obj is None: return

Copy link
Member

@lbdreyer lbdreyer left a comment

Choose a reason for hiding this comment

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

Brilliant! Thanks for all your work on this PR @schlunma !

I have no other comments so this can go in!

@lbdreyer lbdreyer merged commit 4900d36 into SciTools:main Mar 10, 2023
@schlunma schlunma deleted the smarter_weights branch March 10, 2023 15:12
tkknight added a commit to tkknight/iris that referenced this pull request Apr 22, 2023
* upstream/main: (23 commits)
  Lockfiles and pydata-sphinx-theme fix (SciTools#5188)
  Allow smarter weights (cubes, coordinates, cell measures, or ancillary variables) for aggregation (SciTools#5084)
  removed cell measure mask check and error (SciTools#5181)
  Updated environment lockfiles (SciTools#5177)
  Lazy weighted RMS calculation (SciTools#5017)
  Add coverage badge to README.md (SciTools#5176)
  Add coverage testing (SciTools#4765)
  Whats new updates for v3.4.1 .
  NetCDF thread safety take two (SciTools#5095)
  Updated environment lockfiles (SciTools#5163)
  Plugin support (SciTools#5144)
  Expand scope of common contributor links (SciTools#5159)
  Replace apparently retired UDUNITS documentation link. (SciTools#5153)
  [pre-commit.ci] pre-commit autoupdate (SciTools#5150)
  Fixing typo's in Gitwash. (SciTools#5145)
  add readme #showyourstripes (SciTools#5141)
  [pre-commit.ci] pre-commit autoupdate (SciTools#5143)
  Iris ❤ Xarray docs page. (SciTools#5025)
  [pre-commit.ci] pre-commit autoupdate (SciTools#5136)
  Updated citation (SciTools#5116)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Smarter weighted aggregation
5 participants