-
Notifications
You must be signed in to change notification settings - Fork 45
Add Grid preservation to groupby, groupby_bin & resample xarray operations
#1308
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
Conversation
== NO GRID OBJECT this works: == |
|
The issue in #1220 is due to the |
Right, I put what you mentioned in the PR description. The checks here should still be fine as a safety mechanism. |
Oops I must have missed that! |
philipc2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. I'm going to take a deeper look on Monday when I'm back from PTO this week.
philipc2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just ran the example provided in #1220, and the code executes, however the final UxDataset is no longer linked to a Grid, with .uxgrid being None
|
Actually, looks like the |
philipc2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May you update the title of this PR to reflect the changes? It helps when creating releases since they are automatically generated from the PR titles.
…ling, The implementation works with objects used in climate data, Time dimensions are correctly reduced by resampling operations
Good observation and I have been thinking about an alternate approach that might be better - How about wrapping the xarray accessor objects (GroupBy/Bin, Resample, |
…entation with accessor classes that wrap xarray operations. Removed inline proxy classes and the _wrap_callable_attr helper in favor of a centralized accessor module. Preserve Key Attribute: This ensures the uxgrid attribute is preserved through all groupby, resample, rolling, coarsen, weighted, and cumulative operations. Improve Code Quality: The new approach provides better maintainability, cleaner separation of concerns, and easier extensibility for future accessor methods.
Please review this now, I made changes throughout and this implementation look way better. |
philipc2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! A few suggestions:
- Created new test/test_accessors.py for all groupby/resample/rolling/etc tests - Removed accessor tests from test_dataset.py to keep it focused on core Dataset functionality - Fixed import sorting in accessors.py (pre-commit)
Sorry for the delayed response! Seems like @rajeeja made some changes since this ping. Should I still have a look, @philipc2 ? |
Yes please! I think two reviews are needed for the scope of this PR. |
philipc2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments.
Co-authored-by: Philip Chmielowiec <67855069+philipc2@users.noreply.github.com>
8f0eb38 to
806a539
Compare
Sure! Give me some time, maybe until tomorrow, I will give this a review before the weekend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please find my thoughts about the Grid preservation and cftime handling below:
- I might really be lost in the code, but shouldn't we just override the xarray functions, wherever there is a loss of the Grid object, and simply add back the
uxgridto the data array/set object to be returned? I think that might be a much simpler and adhoc solution, and that way I don't think we should be concerned about_repr_.
I'd imagine this'd look something like (for UxDataset):
def resample(self, ...):
xrds = self.resample(...)
uxds = uxarray.core.dataset.UxDataset(xrds, uxgrid=self.uxgrid)
return uxds
- Not sure about the cftime handling since even
xarraydoes not work on the dataset pointed out in the #1220 . Didn't look into those aprts of this PR yet either. Nevertheless, if we can get it to work with uxarray easily, maybe it is worth handling here.
The accessor pattern is necessary because:
Direct override only handles the first call, not the chained operations. The accessor wraps the entire
You're right that xarray itself has issues with cftime in certain cases. The current implementation passes |
Never mind, I made an assumption, too early, as if Now understanding this, I will give your code a second review.
Got it; thanks for clarifying! |
philipc2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work!
UxArray crashes with an AttributeError: 'NoneType' object has no attribute '_ds' when attempting to display the HTML representation of a UxDataset that has lost its grid reference during certain operations, particularly groupby operations.
added null checking in
_obj_repr_with_grid()and overloaded groupby and resample methods to preserve uxgrid after operations.test cases added to test these chages.
cftime, I thought would be handle by xarray, but was causing issues:
We now detecti cftime objects, extract year/month information directly from them and perform manual binning without requiring conversion to pandas datetime