Skip to content

Conversation

@msteckle
Copy link
Collaborator

@msteckle msteckle commented Nov 12, 2025

I'm submitting a PR but you shouldn't merge it for now because I just want to chat about it first. I had some questions I want to write down:

  • For these transforms, we're returning the ds if it doesn't meet some requirement rather than failing. I know you said you wanted it to be hard to fail, but now I would think the user is expecting to have transformed the data but they don't know that it actually didn't transform, is that right?
  • For integration; caveat: I don't have really any calculus knowledge at all, so I was basically having to look up what integration was and why we should use it instead of just regular old summation. I've got these reasons:
    • Not regularly spaced data (so stretched lat/lon, irregular time steps, etc)
    • Maintaining the physical units
  • Then I found that xarray has .integrate() and I was wondering why you did var.weighted(msr.fillna(0))
    • Then I got stumped on what the general .integrate() function does; do we want a generalized integrate function that still works if a user supplies some other mystical dimension besides lat/lon/time/depth? If so, users could just specify trapezoidal (xr.integrate) or weighted sum (what you used in the code)

Anyway, that's a lot. I'm basically not seeing why we need an integrate() base class when it just uses one of the child classes (integrate_time, integrate_space, integrate_depth) and doesn't actually do anything on it's own.

I also haven't tested this since I wanted to discuss first.

@msteckle msteckle self-assigned this Nov 12, 2025
@msteckle msteckle requested a review from nocollier as a code owner November 12, 2025 00:53
@codecov
Copy link

codecov bot commented Nov 12, 2025

Codecov Report

❌ Patch coverage is 0% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.00%. Comparing base (62bf337) to head (e377517).

Files with missing lines Patch % Lines
ilamb3/transform/integrate.py 0.00% 32 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #99      +/-   ##
==========================================
- Coverage   69.67%   69.00%   -0.67%     
==========================================
  Files          37       38       +1     
  Lines        3307     3339      +32     
==========================================
  Hits         2304     2304              
- Misses       1003     1035      +32     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

"""
Select a dim or dim range of the input dataset, if the dimension is found.
"""
try:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you do not need this block at all.

dim_name = dset.get_dim_name(ds, self.dim)
except KeyError: # so if the user implements integrate but it hits this error, it'll still pass thru even tho it's wrong
return ds
if dim_name == "time":
Copy link
Contributor

Choose a reason for hiding this comment

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

if self.dim == "time":   # we configured this integrate for time
  if not dset.is_temporal(ds): return ds  # but the ds isn't temporal
  ds[self.varname] = dset.integrate_time(ds, self.varname, mean=self.mean)

return ds
if dim_name == "time":
ds[self.varname] = dset.integrate_time(ds, self.varname, mean=self.mean)
elif dim_name == "depth":
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto the others

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.

3 participants