Skip to content

cumulative_integrate() method #5153

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

Merged
merged 6 commits into from
May 1, 2021

Conversation

johnomotani
Copy link
Contributor

@johnomotani johnomotani commented Apr 13, 2021

Provides the functionality of scipy.integrate.cumulative_trapezoid.

  • Tests added
  • Passes pre-commit run --all-files
  • User visible changes (including notable bug fixes) are documented in whats-new.rst

Provides the functionality of scipy.integrate.cumulative_trapezoid.
@dcherian
Copy link
Contributor

There's a cumtrapz in https://github.com/fujiisoup/xr-scipy/blob/master/xrscipy/integrate.py. Does that help?

@johnomotani
Copy link
Contributor Author

There's a cumtrapz in https://github.com/fujiisoup/xr-scipy/blob/master/xrscipy/integrate.py. Does that help?

I did a quick check and it seems like applying scipy.integrate.cumtrapz to a dask array returns a numpy array, whereas the duck_array_ops.cumulative_trapezoid that I copied from duck_array_ops.trapz does return a dask array (so I assume it must not force a compute?). Maybe @fujiisoup has thoughts though?

BTW the test failures are in test_cftimeindex.py so I assume they're unrelated to these changes.

@fujiisoup
Copy link
Member

There's a cumtrapz in https://github.com/fujiisoup/xr-scipy/blob/master/xrscipy/integrate.py. Does that help?

Now most of xr-scipy functionalities are already implemented in xarray and also I couldn't take time to maintain this package.

I think basic functionalities would be better to be integrated into xarray itself and cumulative_trapezoid would be a good candidate, as integrate is already there.

The implementation looks good to me.
I didn't find any edge cases where duck_array_ops.cumulative_trapezoid behaves differently from scipy.integrate.cumtrapz.

@max-sixty
Copy link
Collaborator

If we had a .expanding method (we only have .rolling at the moment), then I guess this could be .expanding(dim).integrate(). Is that right?

(but given we don't yet, no need to delay this)

@max-sixty
Copy link
Collaborator

Great — shall we merge?

@shoyer
Copy link
Member

shoyer commented Apr 25, 2021

This is definitely welcome functionality! My only question is whether it should be a flag in the existing integrate method or a new method altogether, e.g., cumulative_integrate?

I would lean towards a new method because the function signature is different (not removing a dimension).

Since the dimensions of the returned arrays are different for cumulative
and total integrals, makes more sense for them to be separate methods.
@johnomotani
Copy link
Contributor Author

The vote seems to be for a separate cumulative_integrate method - I've made that change.

@johnomotani johnomotani changed the title Option to have integrate() return a cumulative integral cumulative_integrate() method May 1, 2021
@max-sixty
Copy link
Collaborator

Thanks a lot @johnomotani !

@max-sixty max-sixty merged commit 04acabb into pydata:master May 1, 2021
@johnomotani johnomotani deleted the integrate-cumulative branch May 2, 2021 10:34
@johnomotani johnomotani restored the integrate-cumulative branch May 2, 2021 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants