Skip to content

Conversation

@zklaus
Copy link

@zklaus zklaus commented Feb 25, 2019

Closes #3280

Signed-off-by: Klaus Zimmermann [email protected]

@pp-mo
Copy link
Member

pp-mo commented Feb 27, 2019

I haven't had time to check code in depth yet,
but the idea is a really worthwhile + exciting ! 👍 🚀

Some tests for the new functionality will be wanted.

P.S. We just fixed the CI header tests, so sorry about that !

@zklaus
Copy link
Author

zklaus commented Feb 27, 2019

Thanks! Yes, I am going through the checklist now and will add tests and whatsnew and whatnot :).

In fact, this is the only reason why this is marked as WIP, the code itself is considered finished and of course works for me.

Maybe to ease reading it: I completely kept the code for the non-lazy version, just put it in the else branch of an if statement guarding the lazy version in the exact same way as in the collapsed method.

A more aggressive version is possible were also the non-lazy part is implemented in the same way as the lazy part with only lazy_aggregate replaced by aggregate and da.stack replaced by np.stack.

@bjlittle bjlittle self-assigned this Jun 5, 2019
@bjlittle bjlittle added this to the v2.3.0 milestone Jun 5, 2019
@zklaus zklaus force-pushed the lazy_aggregated_by branch from 58d5c30 to 529e980 Compare July 11, 2019 11:48
@zklaus zklaus changed the title WIP: Adds lazy version of aggregated_by routine to Cube. Adds lazy version of aggregated_by routine to Cube. Jul 12, 2019
@zklaus
Copy link
Author

zklaus commented Jul 12, 2019

Ok, @bjlittle, @pp-mo, I think this is ready.

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 your work on this @zklaus ! This is a really nice, clean addition!

I just have a comment on the testing of 2D coordinates

@pp-mo pp-mo assigned pp-mo and unassigned bjlittle Sep 30, 2019
@zklaus
Copy link
Author

zklaus commented Sep 30, 2019

@pp-mo, @lbdreyer thanks for your help. I removed the spanning coordinates.

@pp-mo
Copy link
Member

pp-mo commented Sep 30, 2019

@zklaus great !
Thanks for this really neat improvement.

@pp-mo pp-mo merged commit bed6743 into SciTools:master Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add lazy aggregated_by

4 participants