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

Collapse mean points #3029

Closed
wants to merge 12 commits into from
Closed

Conversation

duncanwp
Copy link
Contributor

Follow up on #3008 - Adding support for partial collapse of multi-dimensional coordinates and now taking the mean of the points from the bounds

(I can rebase this once/if #3028 is merged).

@pelson
Copy link
Member

pelson commented May 31, 2018

I've merged #3028 if you'd like to rebase.
I support this change, but I do recognise that one could consider this to be a breaking/major change.

Relevant threads:

Specifically, @pp-mo commented that he considers this to be a breaking change: #3008 (comment)

@duncanwp
Copy link
Contributor Author

OK, rebased now.

What's the process with breaking changes? Could we squeeze a warning into 2.1 that this will change in 2.2?

@corinnebosley
Copy link
Member

Hi @duncanwp, I'm afraid a warning won't let us make a breaking change in a minor version (aside from the fact that users get so many warnings they all get ignored anyway, it's just plain against the rules).

What I can do is put a label on this so that we are ready to pop it in the next major release and we don't forget about it.

It will need rebasing (again, sorry...), but it might be worth saving that until we're ready to merge it.

@corinnebosley corinnebosley added this to the v3.0.0 milestone Oct 15, 2018
@@ -1219,7 +1219,7 @@ def serialize(x):
# Calculate the bounds and points along the right dims
bounds = al.stack([item.min(axis=dims_to_collapse),
item.max(axis=dims_to_collapse)]).T
points = al.array(bounds.sum(axis=-1) * 0.5, dtype=self.dtype)
points = item.mean(axis=dims_to_collapse, dtype=self.dtype)
Copy link
Member

Choose a reason for hiding this comment

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

There was a question at #3008 about whether multiple collapses are commutative. It looks like by averaging item here, which is the bounds of the input coord where available, we do lose commutativity. E.g.

mycoord = iris.coords.AuxCoord([[1, 2], [10, 11], [100, 101]], long_name='foo')
print(mycoord.collapsed())
print(mycoord.collapsed(0).collapsed())
print(mycoord.collapsed(1).collapsed())

gives:

AuxCoord(array([37]), bounds=array([[  1, 101]]), standard_name=None, units=Unit('1'), long_name='foo')
AuxCoord(array([51]), bounds=array([[  1, 101]]), standard_name=None, units=Unit('1'), long_name='foo')
AuxCoord(array([37]), bounds=array([[  1, 101]]), standard_name=None, units=Unit('1'), long_name='foo')

Is this the intended behaviour, or should the new points be averaged from the input points?

@rcomer
Copy link
Member

rcomer commented Jun 13, 2019

Just some thoughts that occurred to me while thinking about this change. Feel free to answer/argue/ignore me as you see fit. I appreciate I've stumbled in somewhat late in the day!

  • If we change the treatment of points in collapsed, should we also change it in aggregated_by for consistency (I don't know much about rolling_window, but maybe that too)?

  • What happens if a weighted aggregator is passed? Should the points averaging use a weighted mean?

@bjlittle
Copy link
Member

@duncanwp We're not going to able to get this in for iris 3.0 :-(

Our plan is to add this a into iris v3.x but as an opt-in future, which will then become the default behaviour for iris 4.0

@rcomer rcomer self-assigned this Aug 20, 2020
@rcomer rcomer removed their assignment Aug 20, 2020
@bjlittle bjlittle self-assigned this Oct 1, 2020
@bjlittle bjlittle added the Peloton 🚴‍♂️ Target a breakaway issue to be caught and closed by the peloton label Oct 1, 2020
@bjlittle
Copy link
Member

@duncanwp Sadly this PR has been stalled for almost 2 and a half years now.

Rather than let it linger still longer, I'm going to close this PR, which is not something that I really want to do. Personally, I'd rather see it merged and its benefit utilised by the community.

However, if you have the time and motivation, then please re-open this PR and we'll take it from there. Hopefully we can get it across the line. Thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Peloton 🚴‍♂️ Target a breakaway issue to be caught and closed by the peloton Release: Minor Status: Stalled
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants