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

ENH: Add cumulative integration function. #3508

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

DWesl
Copy link

@DWesl DWesl commented May 21, 2024

Description Of Changes

Add a unit-aware cumulative integration function.
Won't be as fast as the xarray cumulative integration function,
but I'll be less likely to forget the units.

Checklist

  • Closes #xxxx
  • Tests added
  • Fully documented

@DWesl DWesl requested a review from a team as a code owner May 21, 2024 12:16
@DWesl DWesl requested review from dcamron and removed request for a team May 21, 2024 12:16
@dopplershift
Copy link
Member

Initial thoughts:

  • If we're adding cumulative_integrate, we should also have an integrate
  • Unsure if something so generic is really a good fit for MetPy, but at the same time we are at the nexus of providing calcs that handle units on xarray
  • Might be easier to make this a thin wrapper around cumulative_trapezoid from scipy.integrate? Tossing a preprocess_and_wrap decorator on the function should do all the xarray work, and then just need to deal with units as appropriate internally.

@DWesl
Copy link
Author

DWesl commented May 21, 2024

Initial thoughts:

  • If we're adding cumulative_integrate, we should also have an integrate

I wanted to get this working before trying for integrate, which would be a simple change. I had thought I was making a PR on my own fork, so I could get test runs from a machine that doesn't hang somewhere in collection.

  • Unsure if something so generic is really a good fit for MetPy, but at the same time we are at the nexus of providing calcs that handle units on xarray

If it helps, it's kind of an inverse of mpcalc.first_derivative. On the other hand, XArray already has a cumulative_integrate method, and wrapping that to deal with the units would be simple.

EDIT: Just noticed np.trapz(pint.Quantity, pint.Quantity) returns a Quantity, which simplifies things. scipy.integrate.cumulative_trapezoid does not. I'm not sure if SciPy uses the NumPy machinery to notice a wrapper.

  • Might be easier to make this a thin wrapper around cumulative_trapezoid from scipy.integrate? Tossing a preprocess_and_wrap decorator on the function should do all the xarray work, and then just need to deal with units as appropriate internally.

If preprocess_and_wrap is public, another option would be a gallery example using that to calculate PWAT or liquid water path with units.

Copy link
Author

@DWesl DWesl left a comment

Choose a reason for hiding this comment

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

Suggestions to fix NameError in tests and lint errors for test docstrings.

src/metpy/calc/tools.py Outdated Show resolved Hide resolved
src/metpy/calc/tools.py Outdated Show resolved Hide resolved
tests/calc/test_calc_tools.py Show resolved Hide resolved
tests/calc/test_calc_tools.py Show resolved Hide resolved
tests/calc/test_calc_tools.py Show resolved Hide resolved
DWesl added 3 commits May 21, 2024 16:28
That's what I get for not paying attention when borrowing implementation while changing names.

Also fix some lint errors.
Docstrings are double quotes in the rest of the file.
tests/calc/test_calc_tools.py Fixed Show fixed Hide fixed
tests/calc/test_calc_tools.py Fixed Show fixed Hide fixed
tests/calc/test_calc_tools.py Fixed Show resolved Hide resolved
tests/calc/test_calc_tools.py Fixed Show resolved Hide resolved
@DWesl
Copy link
Author

DWesl commented Jun 5, 2024

Initial thoughts:

  • Unsure if something so generic is really a good fit for MetPy, but at the same time we are at the nexus of providing calcs that handle units on xarray

Looks like cf-xarray would also be an option, since they already have a differentiate:
https://cf-xarray.readthedocs.io/en/latest/generated/xarray.Dataset.cf.differentiate.html

  • Might be easier to make this a thin wrapper around cumulative_trapezoid from scipy.integrate? Tossing a preprocess_and_wrap decorator on the function should do all the xarray work, and then just need to deal with units as appropriate internally.

I think I have that working now.

  • If we're adding cumulative_integrate, we should also have an integrate

The preprocess_and_wrap decorator doesn't like the result of the function having fewer dimensions than the input to the function. I'm not entirely sure how to handle that.

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.

None yet

2 participants