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

Weighted mean in cube.aggregated_by #3290

Closed
ledm opened this issue Mar 6, 2019 · 9 comments
Closed

Weighted mean in cube.aggregated_by #3290

ledm opened this issue Mar 6, 2019 · 9 comments

Comments

@ledm
Copy link

ledm commented Mar 6, 2019

Hi,

is it possible to use weights to calculate the mean while using cube.aggregated_by.

For instance, I have a cube with several years of monthly data, and I want to produce an annual mean. I have previously used:

cube.aggregated_by('year', iris.analysis.MEAN)

However, this means that the shorter months have the same weighting as the longer months. Is there a weight to weight this calculation by some given weights. (Lets assume that I've calculated them elsewhere!)
ie:

cube.aggregated_by('year', iris.analysis.MEAN, weights=time_weights)

Thanks!

@pp-mo
Copy link
Member

pp-mo commented Mar 6, 2019

Absolutely yes, it does support a 'weights' keyword
see here

@ledm
Copy link
Author

ledm commented Mar 6, 2019

Hi Patrick!

I'm seeing the error message:

 ValueError: Invalid Aggregation, aggregated_by() cannot use weights.

Which seems to come from:

            # We can't handle weights
            if isinstance(aggregator, iris.analysis.WeightedAggregator) and \
                    aggregator.uses_weighting(**kwargs):
>               raise ValueError('Invalid Aggregation, aggregated_by() cannot use'
                                 ' weights.')

in iris/cube.py:3363 and we're in iris version 2.2.0.

It probably isn't the culprit, but the function that you're linking is the iris.analysis.MEAN function, not the aggregated_by function that I'm using. Is there perhaps a problem with the way I'm passing the weights to aggregated_by?

@pp-mo
Copy link
Member

pp-mo commented Mar 6, 2019

Sorry, my bad !! 👎
I was confusing the functions of the aggregator with the stats routine (aggregated_by instead of collapsed).

@bjlittle
Copy link
Member

bjlittle commented Mar 6, 2019

In principle, this seems possible...

I think it might be worthwhile taking a step back here and holistically rationalising how aggregation and collapse works. Unifying behaviour for use of weights (where appropriate for the statistic) consistently would be a good win - plus there is some technical debt for us to purge, as there is a major overlap between aggregation and collapse that we should take advantage of; I've been itching to do that for years.

The topic of operator laziness (#3280) is also relevant here... and that needs a wee bit more thought. You'd certainly throw away coordinate laziness, but it seems reasonable to expect to keep the data lazy.

@pp-mo
Copy link
Member

pp-mo commented Mar 7, 2019

Unifying behaviour for use of weights ... consistently

  • ... overlap between aggregation and collapse that we should take advantage of
  • ... operator laziness ... throw away coordinate laziness, but ... keep the data lazy

👍 great summary!

I might just add, can we also consider getting rid of the "two-speed" separate lazy/real implementations ? (as in #2418)

@pp-mo pp-mo added the votable label May 1, 2019
@github-actions
Copy link
Contributor

In order to maintain a backlog of relevant issues, we automatically label them as stale after 500 days of inactivity.

If this issue is still important to you, then please comment on this issue and the stale label will be removed.

Otherwise this issue will be automatically closed in 28 days time.

@github-actions github-actions bot added the Stale A stale issue/pull-request label Aug 11, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2021

This stale issue has been automatically closed due to a lack of community activity.

If you still care about this issue, then please either:

  • Re-open this issue, if you have sufficient permissions, or
  • Add a comment pinging @SciTools/iris-devs who will re-open on your behalf.

@rcomer
Copy link
Member

rcomer commented May 2, 2022

This was implemented in #4589

@ledm
Copy link
Author

ledm commented May 3, 2022

Great work, thanks to the iris team! This will be very useful in the future. Now if I can only remember what I was trying to do back in March 2019, lol.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants