Cumulative examples

[patrick-naylor]

Here I am trying to add docstring example to the cumulative functions. I did this by basically copying the method used for the reduction functions. Not sure at all if I did this correctly so i'm marking it a draft

• Tests added
• User visible changes (including notable bug fixes) are documented in whats-new.rst
• New functions/methods are listed in api.rst

[Illviljan]

Very nice, this is something that's been on the TODO list! :)

I believe we wanted to rename generate_reductions.py to generate_aggregations.py so cumsum et al could be included and generated there as well. Is there a lot of work for you if try to merge these into that one?

[patrick-naylor]

@Illviljan That is definitely something I could do. Are there any other methods I should be including in this?

[Illviljan]

Right now, I think cumsum and cumprod is enough. numpy-groupies has a few more examples that I suppose we could support in the future.

[patrick-naylor]

Great I'll start working on that. Shouldn't take too long

[dcherian]

Thanks @patrick-naylor !

Instead of using Dataset.reduce I think we want something like

def cumsum(..., dim):
	return xr.apply_ufunc(
	    np.cumsum if skipna else np.nancumsum,
		obj,
	    input_core_dims=[dim],
	    output_core_dims=[dim],
        kwargs={"axis": -1},
    )
    # now transpose dimensions back to input order

to fix #6528.

At the moment, this should also work on GroupBy objects quite nicely.

[patrick-naylor]

Thanks @dcherian,
I'll try to work that in.
Is there a particular reason why there is no cumprod for GroupBy objects?

[dcherian]

Is there a particular reason why there is no cumprod for GroupBy objects?

Nope. Just wasn't added in :)

[patrick-naylor]

def cumsum(..., dim):
	return xr.apply_ufunc(
	    np.cumsum if skipna else np.nancumsum,
		obj,
	    input_core_dims=[dim],
	    output_core_dims=[dim],
        kwargs={"axis": -1},
    )
    # now transpose dimensions back to input order

I'm running into an issue with variables without the core dimensions. Would it be better to do a work around in cumsum or in apply_unfunc like you mentioned in #6391

[headtr1ck]

I'm running into an issue with variables without the core dimensions. Would it be better to do a work around in cumsum or in apply_unfunc like you mentioned in #6391

While this is definitely worth an improvement, it is quite out of scope for this PR.
Can you define Datasets such that this problem does not occur?

[patrick-naylor]

I've merged the cumulative and reduction files into generate_aggregations.py and _aggregations.py. This uses the original version of reductions with an additional statement on the dataset methods that adds the original coordinates back in.

Using apply_ufunc and np.cumsum/cumprod has some issues as it only finds the cumulative across one axis which makes iterating through each dimension necessary. This makes it slower than the original functions and also causes some problems with the groupby method.

Happy for any input on how the method using apply_ufunc might be usable or on any ways to change the current method.

I'm getting a few issues I don't quite understand:

• When running pytest on my local repository I get no errors but it's failing the checks here with a NotImplementedError
• Black is having an issue with some of the strings in generate_aggregations. It's saying it cannot parse what should be valid code.

Thanks!

[Illviljan]

I don't think you have flox installed, if it's not installed the code will take the old path.
Do conda install flox and I think you'll get the NotImplementedError. Then you maybe have to change the default settings in cumsum so flox is not used.

[dcherian]

Thanks for taking this on @patrick-naylor ! This is a decent-sized project!

Using apply_ufunc and np.cumsum/cumprod has some issues as it only finds the cumulative across one axis which makes iterating through each dimension necessary.

np.cumsum only supports an integer axis so this is OK?

flox doesn't support cumsum at the moment (xarray-contrib/flox#91) so we can delete that bit and just have one code path.

[dcherian]

Thanks @patrick-naylor , this is a major step forward.

I think it might be better to take a step back and figure out Dataset.cumsum using apply_ufunc that passes tests. Then we can update the generators with that code, iterating using the generators is going to be frustrating ;)

[dcherian]

We shouldn't be supporting axis, Xarray uses dim instead.

[patrick-naylor]

I initially thought this was just a documentation error with the old cumsum/prod docs where they had axis as a parameter but the current version of dataarray.cumsum/dataarray.cumprod does support axis input. I was worried that removing it may break some users' code. Is this something we should depreciate, leave in, or just outright remove?

I also just realized that I had axis as an additional kwarg for the dataset methods not dataarray methods which is my mistake.

[dcherian]

old cumsum/prod docs where they had axis

We've been removing these so IMO its OK. I think it was an issue with the older reduction functions too

[dcherian]

THis median bit should not be needed

[dcherian]

In general, these strings were tweaked to make sure pre-commit was happy so its unlikely these changes are necessary.

[dcherian]

Note we previously had numeric_only=True though I guess it may not be needed here.

[Illviljan]

I added it now, or should I revert?

[dcherian]

LGTM. Let's open an issue to add tests when we relax it.

[dcherian]

Thanks @patrick-naylor and @Illviljan . This is an amazing improvement!

[Illviljan]

@patrick-naylor, feel free to try out a better default example if you want.

[patrick-naylor]

Thanks @Illviljan, @dcherian, and @keewis so much for the help.