Skip to content

Conversation

@DWesl
Copy link
Contributor

@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
Contributor 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
Contributor 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.

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.
@DWesl
Copy link
Contributor 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.

@DWesl
Copy link
Contributor Author

DWesl commented Oct 22, 2025

pint-xarray handles this now:

import xarray as xr, pint_xarray
ds = xr.open_dataset(...)
integral = ds.pint.quantify().integrate(...).pint.dequantify("~C")

I'll check cumulative_integrate in a bit

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.

2 participants