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

Support Python 3.12 and Migrate to Xarray DataTree #1419

Merged
merged 30 commits into from
Jan 1, 2025

Conversation

ctuguinay
Copy link
Collaborator

@ctuguinay ctuguinay commented Dec 26, 2024

Still in draft.

Resolves #1405, #1408, #1418.

Following the DataTree Migration Guide detailed here: https://github.com/pydata/xarray/blob/main/DATATREE_MIGRATION_GUIDE.md.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov-commenter
Copy link

codecov-commenter commented Dec 30, 2024

Codecov Report

Attention: Patch coverage is 36.36364% with 7 lines in your changes missing coverage. Please review.

Project coverage is 72.92%. Comparing base (9f56124) to head (b1f147f).
Report is 163 commits behind head on main.

Files with missing lines Patch % Lines
echopype/echodata/echodata.py 40.00% 3 Missing ⚠️
echopype/echodata/combine.py 0.00% 2 Missing ⚠️
echopype/echodata/widgets/utils.py 33.33% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1419       +/-   ##
===========================================
- Coverage   83.52%   72.92%   -10.60%     
===========================================
  Files          64       37       -27     
  Lines        5686     4396     -1290     
===========================================
- Hits         4749     3206     -1543     
- Misses        937     1190      +253     
Flag Coverage Δ
unittests 72.92% <36.36%> (-10.60%) ⬇️

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.

@ctuguinay
Copy link
Collaborator Author

ctuguinay commented Dec 30, 2024

I commented out TestEchoData in echopype/tests/echodata/test_echodata.py since it was failing and it had most likely something to do with the backward compatibility issue #1420 (since it was the open_converted parent-child alignment that was failing).

@ctuguinay ctuguinay marked this pull request as ready for review December 31, 2024 01:36
@ctuguinay ctuguinay requested a review from leewujung December 31, 2024 01:37
@ctuguinay
Copy link
Collaborator Author

This should be ready for review @leewujung

Comment on lines 151 to +159
"NMEA_datagram": (
["time1"],
["nmea_time"],
raw_nmea,
{"long_name": "NMEA datagram"},
)
},
coords={
"time1": (
["time1"],
"nmea_time": (
["nmea_time"],
Copy link
Member

Choose a reason for hiding this comment

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

How about time_nmea to be consistent with the noun-modifier pattern that's used elsewhere?

Copy link
Member

Choose a reason for hiding this comment

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

Ugh just saw that there's ping_time that deviates from this pattern, and it would be odd to make it time_ping. Personally I feel it is cleaner for the other ones to be all like time_X...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh huh I never thought about it that way. I personally see them both as a standalone/compound noun since 'ping' + 'time' refer to different things but together form this one distinct thing and 'ping time' could be operated on like "short + 'ping time'", and 'nmea time' I feel could be operated on similarly.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see what you mean, so ping time is when a ping is issues, and nmea is when an nmea sentence is issues. I agree with that. Perhaps we can just keep it this way here. We will at some point need a larger breaking change to keep up with convention changes once more feedback from the community is discussed, but that'll be at least a few months down the road.

Copy link
Member

@leewujung leewujung left a comment

Choose a reason for hiding this comment

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

@ctuguinay : Thanks for the investigation and fixes! I think this is ready to be merged once the time name is settled. My comment about __get_dataset is probably better addressed separately, if needed.

@ctuguinay ctuguinay merged commit b11cb0e into OSOceanAcoustics:main Jan 1, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data format Anything about data format infrastructure
Projects
Status: Done
3 participants