Switch feature detection to use xarray internally #354

freemansw1 · 2023-10-11T15:23:10Z

This is a draft PR that contains my work switching feature detection to use xarray internally, using many of the concepts (and some of the code) from #275. This code uses entirely xarray and pandas for processing feature detection, and removes all iris calls; instead using our internal conversion utilities to convert input iris data to xarray (which should always work, unlike xarray->iris. Further, this change should allow users to provide their data as an xarray.DataArray.

This PR will remain in draft form until at least #293 is merged and intentionally includes those changes.

More tests (particularly around times on different OS versions) should be added, but I wanted to get this into a form that we could discuss.

# Conflicts: # tobac/feature_detection.py

…_xarray_work_fd_julia_avgs # Conflicts: # tobac/feature_detection.py # tobac/utils/__init__.py # tobac/utils/general.py

…ulia_avgs

…_xarray_work_fd_julia_avgs # Conflicts: # tobac/utils/__init__.py # tobac/utils/general.py

…y_work_fd_julia_avgs # Conflicts: # tobac/utils/general.py

…y_work_fd_julia_avgs # Conflicts: # tobac/feature_detection.py # tobac/utils/internal/xarray_utils.py

codecov · 2023-11-09T22:07:58Z

Codecov Report

Attention: Patch coverage is 78.20513% with 68 lines in your changes missing coverage. Please review.

Project coverage is 62.27%. Comparing base (5f82097) to head (bbb6b67).
Report is 46 commits behind head on RC_v1.6.0.

Files with missing lines	Patch %	Lines
tobac/utils/internal/iris_utils.py	63.63%	52 Missing ⚠️
tobac/utils/internal/xarray_utils.py	88.88%	14 Missing ⚠️
tobac/utils/general.py	85.71%	2 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##           RC_v1.6.0     #354      +/-   ##
=============================================
+ Coverage      60.91%   62.27%   +1.36%     
=============================================
  Files             23       23              
  Lines           3541     3690     +149     
=============================================
+ Hits            2157     2298     +141     
- Misses          1384     1392       +8

Flag	Coverage Δ
unittests	`62.27% <78.20%> (+1.36%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

…y_work_fd_julia_avgs

JuliaKukulies · 2024-03-22T14:28:24Z

Great, thanks for making the revisions. I've had a look through the changes and tested the code with both xarray and iris locally and have no issues to issues. Should we start an RC_v1.6.0 branch to merge this into?

Yes, we will need to do that. I'll wait until we have to though to minimize the amount of time we have to maintain separate branches. We still need one more reviewer, @JuliaKukulies or @kelcyno are you up for it?

I can do the second review within the next couple of days!

I also started a RC_v1.6.0 branch to merge this PR into. We can wait until the release of v1.5.3 to update the branch before this PR is merged. But I will try to review asap :)

…y_work_fd_julia_avgs # Conflicts: # tobac/analysis.py # tobac/feature_detection.py

JuliaKukulies

Thanks a lot for this pR @freemansw1! This is seems like a great step towards the xarray transition.

It all looks good to me besides two small comments. And I have tested this on two real data sets which works fine and without any major issues. However, the only thing that does not seem to work is is passing the argument preserve_iris_datetime_type to feature_detection_multithreshold(). The dtypes for time are different in the output feature dataframe no matter if I set preserve_iris_datetime_type to False or True. They are always objects/dtype('O') when I input an iris cube and datetime64/dtype('<M8[ns]') when I input an xarray.

tobac/analysis.py

tobac/utils/bulk_statistics.py

freemansw1 · 2024-05-09T21:25:04Z

Ok, @JuliaKukulies, I think I've addressed your concerns. Ready for your re-review. Hopefully we can get this merged!

JuliaKukulies · 2024-05-10T00:20:24Z

Excellent, @freemansw1! This looks good except for that there is still the issue with preserve_iris_datetime_type which does not result in the same datetime format as when I provide iris input when this parameters is True (see my last comment).

Sorry I had not marked the bug at the actual code locations, so I get that you might have overseen this. I also cannot really figure out where this bug comes from, since everywhere in the code where preserve_iris_datetime_type is used looks like it should work. Could you reproduce this or is this an issue with my test data?

freemansw1 · 2024-05-13T17:56:20Z

Excellent, @freemansw1! This looks good except for that there is still the issue with preserve_iris_datetime_type which does not result in the same datetime format as when I provide iris input when this parameters is True (see my last comment).

Sorry I had not marked the bug at the actual code locations, so I get that you might have overseen this. I also cannot really figure out where this bug comes from, since everywhere in the code where preserve_iris_datetime_type is used looks like it should work. Could you reproduce this or is this an issue with my test data?

Apologies, yeah, I had missed this. I will work to reproduce and try this out this weekend/early next week.

w-k-jones · 2024-05-13T23:27:11Z

I think I've noticed this before, and the issue is to do with calculating the timesteps from cftime format. I think that in the current versions of tobac the time is also returned with an object dtype, but I would be happier to change it to datetime64 in all cases as I currently have to do this for most use cases...

freemansw1 · 2024-06-03T02:27:09Z

Note to self: add_coords for xarray fails when having coordinates to interpolate along dimensions other than x/y.

freemansw1 · 2024-07-17T20:37:44Z

OK, I think I have resolved the datetime issues. It's somewhat of a hacky solution, but basically, xarray only converts to datetime64 if the time received is in Gregorian time. I now check for this and convert back to Gregorian time. For all other calendars (thanks @harrietgilmour for helping with this part), xarray leaves it as an object (which does still break our code, but alas).

…y with time

freemansw1 · 2024-07-18T17:18:40Z

Note to self: add_coords for xarray fails when having coordinates to interpolate along dimensions other than x/y.

I have now also resolved this issue. The solution here results in some decisions that were contrary to the discussion we had at the last dev meeting (notably that len(1) coordinates are still appended to the dataframe), but I think we are otherwise okay. We can talk about this at the dev meeting tomorrow.

freemansw1 · 2024-07-18T17:18:56Z

@w-k-jones and @JuliaKukulies I am now ready for a re-review.

JuliaKukulies · 2024-07-19T19:55:15Z

OK, I think I have resolved the datetime issues. It's somewhat of a hacky solution, but basically, xarray only converts to datetime64 if the time received is in Gregorian time. I now check for this and convert back to Gregorian time. For all other calendars (thanks @harrietgilmour for helping with this part), xarray leaves it as an object (which does still break our code, but alas).

OK, I have tested this again with my data and I thought I understood what you explained in the dev meeting today, but now i am a little confused again :) I basically get:

for xarray input and preserve_iris_datetime_type = True -> datetime64
for xarray input and preserve_iris_datetime_type = False -> datetime64
for iris input and preserve_iris_datetime_type = True -> object (cftime.DatetimeGregorian)
for iris input and preserve_iris_datetime_type = False -> object (cftime.DatetimeGregorian)

Is that what we want?

freemansw1 · 2024-07-22T14:01:03Z

OK, I think I have resolved the datetime issues. It's somewhat of a hacky solution, but basically, xarray only converts to datetime64 if the time received is in Gregorian time. I now check for this and convert back to Gregorian time. For all other calendars (thanks @harrietgilmour for helping with this part), xarray leaves it as an object (which does still break our code, but alas).

OK, I have tested this again with my data and I thought I understood what you explained in the dev meeting today, but now i am a little confused again :) I basically get:

for xarray input and preserve_iris_datetime_type = True -> datetime64

for xarray input and preserve_iris_datetime_type = False -> datetime64

for iris input and preserve_iris_datetime_type = True -> object (cftime.DatetimeGregorian)

for iris input and preserve_iris_datetime_type = False -> object (cftime.DatetimeGregorian)

Is that what we want?

Ok, the first of those three are all correct, but the last one isn't.

JuliaKukulies · 2024-07-22T14:22:22Z

OK, I think I have resolved the datetime issues. It's somewhat of a hacky solution, but basically, xarray only converts to datetime64 if the time received is in Gregorian time. I now check for this and convert back to Gregorian time. For all other calendars (thanks @harrietgilmour for helping with this part), xarray leaves it as an object (which does still break our code, but alas).

OK, I have tested this again with my data and I thought I understood what you explained in the dev meeting today, but now i am a little confused again :) I basically get:

for xarray input and preserve_iris_datetime_type = True -> datetime64

for xarray input and preserve_iris_datetime_type = False -> datetime64

for iris input and preserve_iris_datetime_type = True -> object (cftime.DatetimeGregorian)

for iris input and preserve_iris_datetime_type = False -> object (cftime.DatetimeGregorian)

Is that what we want?

Ok, the first of those three are all correct, but the last one isn't.

Thats what I thought, and we would expect datetime64 for the last one, right? Have you tested this with the data I sent you or another dataset? Sorry to cause more struggle..

freemansw1 · 2024-07-23T14:38:06Z

OK, I think I have resolved the datetime issues. It's somewhat of a hacky solution, but basically, xarray only converts to datetime64 if the time received is in Gregorian time. I now check for this and convert back to Gregorian time. For all other calendars (thanks @harrietgilmour for helping with this part), xarray leaves it as an object (which does still break our code, but alas).

OK, I have tested this again with my data and I thought I understood what you explained in the dev meeting today, but now i am a little confused again :) I basically get:

for xarray input and preserve_iris_datetime_type = True -> datetime64

for xarray input and preserve_iris_datetime_type = False -> datetime64

for iris input and preserve_iris_datetime_type = True -> object (cftime.DatetimeGregorian)

for iris input and preserve_iris_datetime_type = False -> object (cftime.DatetimeGregorian)

Is that what we want?

Ok, the first of those three are all correct, but the last one isn't.

Thats what I thought, and we would expect datetime64 for the last one, right? Have you tested this with the data I sent you or another dataset? Sorry to cause more struggle..

Ok this one was easy- we didn't actually expect the kwarg in feature_detection_multithreshold. I've fixed that.

…y_work_fd_julia_avgs

JuliaKukulies · 2024-07-24T21:32:07Z

OK, I think I have resolved the datetime issues. It's somewhat of a hacky solution, but basically, xarray only converts to datetime64 if the time received is in Gregorian time. I now check for this and convert back to Gregorian time. For all other calendars (thanks @harrietgilmour for helping with this part), xarray leaves it as an object (which does still break our code, but alas).

OK, I have tested this again with my data and I thought I understood what you explained in the dev meeting today, but now i am a little confused again :) I basically get:

for xarray input and preserve_iris_datetime_type = True -> datetime64

for xarray input and preserve_iris_datetime_type = False -> datetime64

for iris input and preserve_iris_datetime_type = True -> object (cftime.DatetimeGregorian)

for iris input and preserve_iris_datetime_type = False -> object (cftime.DatetimeGregorian)

Is that what we want?

Ok, the first of those three are all correct, but the last one isn't.

Thats what I thought, and we would expect datetime64 for the last one, right? Have you tested this with the data I sent you or another dataset? Sorry to cause more struggle..

Ok this one was easy- we didn't actually expect the kwarg in feature_detection_multithreshold. I've fixed that.

Perfect, thanks for looking into this and fixing this. I am happy to approve this now once the latest changes from RC_v1.5.x are merged in

freemansw1 · 2024-07-25T13:51:39Z

OK, changes all merged in and everything. @JuliaKukulies and @w-k-jones are you happy for me to merge into RC_v1.6.0?

JuliaKukulies · 2024-07-25T14:12:14Z

OK, changes all merged in and everything. @JuliaKukulies and @w-k-jones are you happy for me to merge into RC_v1.6.0?

Yes, please go ahead:) !

w-k-jones · 2024-07-25T14:27:37Z

Also happy for this to be merged :)

freemansw1 added 21 commits September 10, 2023 21:37

begun refactor of internal utils

0e2b1c3

add new xarray utils file for xarray functions

f12bd72

continuing to separate out utilities into iris and xarray

e8b8c71

ensure py3.8 compatibility, ensure output types work

a480730

Merge branch 'internal_utils_reorg' into sfreeman_xarray_work

d6cd8cc

# Conflicts: # tobac/feature_detection.py

merging in new changes

3c7404e

Merge branch 'internal_utils_reorg' into sfreeman_xarray_work

612082b

Updates to xarray support in utilities.

94d6da3

Added additional tests to new xarray utilities

a3b5751

switch feature_detection_multithreshold_timestep to xarray

76690d3

first step of switch feature_detection_multithreshold to xarray

110a205

linting for xarray_utils

61329e4

adding new test function to test iris and xarray pathways

c566788

working xarray interpolation now

ae6cf80

adding 2D array interpolation

3dbf497

switch to asserting for almost equal

755b4ea

Merge remote-tracking branch 'julia/feature_statistics' into sfreeman…

1ae63bb

…_xarray_work_fd_julia_avgs # Conflicts: # tobac/feature_detection.py # tobac/utils/__init__.py # tobac/utils/general.py

incorporating julia's statistics code

3170496

adding times to add_coordinates

8dfc1dd

Merge branch 'sfreeman_xarray_work_fd' into sfreeman_xarray_work_fd_j…

d1d48ef

…ulia_avgs

Merge remote-tracking branch 'julia/feature_statistics' into sfreeman…

f642823

…_xarray_work_fd_julia_avgs # Conflicts: # tobac/utils/__init__.py # tobac/utils/general.py

freemansw1 added the xarray transition Part of the transition to xarray support label Oct 11, 2023

Merge remote-tracking branch 'upstream/RC_v1.5.x' into sfreeman_xarra…

cb191b1

…y_work_fd_julia_avgs # Conflicts: # tobac/utils/general.py

freemansw1 mentioned this pull request Nov 8, 2023

Update black integration to say what is wrong and fix python versions #362

Merged

11 tasks

freemansw1 added 3 commits November 9, 2023 16:01

Merge remote-tracking branch 'upstream/RC_v1.5.x' into sfreeman_xarra…

413c999

…y_work_fd_julia_avgs # Conflicts: # tobac/feature_detection.py # tobac/utils/internal/xarray_utils.py

fixes for updates from upstream

034982b

fix literal import in feature detection

0a5f9b7

freemansw1 added this to the Version 1.6 milestone Nov 13, 2023

Merge remote-tracking branch 'upstream/RC_v1.5.x' into sfreeman_xarra…

6c636ce

…y_work_fd_julia_avgs

JuliaKukulies changed the base branch from RC_v1.5.x to RC_v1.6.0 March 22, 2024 16:34

freemansw1 added 2 commits March 27, 2024 09:57

Merge remote-tracking branch 'upstream/RC_v1.6.0' into sfreeman_xarra…

f279584

…y_work_fd_julia_avgs # Conflicts: # tobac/analysis.py # tobac/feature_detection.py

fix for renamed functions

0f4dcee

JuliaKukulies reviewed Apr 11, 2024

View reviewed changes

tobac/analysis.py Outdated Show resolved Hide resolved

tobac/utils/bulk_statistics.py Outdated Show resolved Hide resolved

fixed from Julia's comments

c54d5d5

w-k-jones mentioned this pull request Jun 24, 2024

Floating point error in feature detection breaks interpn #433

Closed

fix preserve_iris_datetime_types for most cases.

c07b1d5

updated add_coordinates_to_features to work with coordinates that var…

cd19a20

…y with time

freemansw1 mentioned this pull request Jul 18, 2024

Fix floating point error in 2d/3d coordinate interpolation #434

Merged

11 tasks

add keyword for preserve_iris_datetime_types to feature detection

7f6930b

Merge remote-tracking branch 'upstream/RC_v1.6.0' into sfreeman_xarra…

bbb6b67

…y_work_fd_julia_avgs

freemansw1 merged commit 0ebee82 into tobac-project:RC_v1.6.0 Jul 25, 2024
21 checks passed

freemansw1 mentioned this pull request Jul 26, 2024

Bulk stats bug fix #437

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch feature detection to use xarray internally #354

Switch feature detection to use xarray internally #354

freemansw1 commented Oct 11, 2023 •

edited

Loading

codecov bot commented Nov 9, 2023 •

edited

Loading

JuliaKukulies commented Mar 22, 2024 •

edited

Loading

JuliaKukulies left a comment

freemansw1 commented May 9, 2024

JuliaKukulies commented May 10, 2024

freemansw1 commented May 13, 2024

w-k-jones commented May 13, 2024 •

edited

Loading

freemansw1 commented Jun 3, 2024

freemansw1 commented Jul 17, 2024

freemansw1 commented Jul 18, 2024

freemansw1 commented Jul 18, 2024

JuliaKukulies commented Jul 19, 2024

freemansw1 commented Jul 22, 2024

JuliaKukulies commented Jul 22, 2024

freemansw1 commented Jul 23, 2024

JuliaKukulies commented Jul 24, 2024

freemansw1 commented Jul 25, 2024

JuliaKukulies commented Jul 25, 2024

w-k-jones commented Jul 25, 2024

Switch feature detection to use xarray internally #354

Switch feature detection to use xarray internally #354

Conversation

freemansw1 commented Oct 11, 2023 • edited Loading

codecov bot commented Nov 9, 2023 • edited Loading

Codecov Report

JuliaKukulies commented Mar 22, 2024 • edited Loading

JuliaKukulies left a comment

Choose a reason for hiding this comment

freemansw1 commented May 9, 2024

JuliaKukulies commented May 10, 2024

freemansw1 commented May 13, 2024

w-k-jones commented May 13, 2024 • edited Loading

freemansw1 commented Jun 3, 2024

freemansw1 commented Jul 17, 2024

freemansw1 commented Jul 18, 2024

freemansw1 commented Jul 18, 2024

JuliaKukulies commented Jul 19, 2024

freemansw1 commented Jul 22, 2024

JuliaKukulies commented Jul 22, 2024

freemansw1 commented Jul 23, 2024

JuliaKukulies commented Jul 24, 2024

freemansw1 commented Jul 25, 2024

JuliaKukulies commented Jul 25, 2024

w-k-jones commented Jul 25, 2024

freemansw1 commented Oct 11, 2023 •

edited

Loading

codecov bot commented Nov 9, 2023 •

edited

Loading

JuliaKukulies commented Mar 22, 2024 •

edited

Loading

w-k-jones commented May 13, 2024 •

edited

Loading