echopype icon indicating copy to clipboard operation
echopype copied to clipboard

Added alpha-version code for new AZFP6 format

Open dash-uvic opened this issue 1 year ago • 10 comments
trafficstars

Added in initial code for the new AZFP6 sensor format for *.azfp files from ASL Environmental services. The data is in a new format and has additional optional GPS information. The XML file is now embedded into the data file. No tests added.

dash-uvic avatar May 29 '24 21:05 dash-uvic

Hey @dash-uvic : Thanks for the contribution! Could you fix the pre-commit errors? Also let us know what files you would like to add to the CI. As always, small files are better!

leewujung avatar Jun 04 '24 06:06 leewujung

Fixed pre-commit errors.

dash-uvic avatar Jun 07 '24 23:06 dash-uvic

Codecov Report

:x: Patch coverage is 87.46667% with 47 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 48.48%. Comparing base (9f56124) to head (2066c95). :warning: Report is 233 commits behind head on main.

Files with missing lines Patch % Lines
echopype/convert/parse_azfp6.py 84.66% 44 Missing :warning:
echopype/convert/set_groups_azfp6.py 97.56% 2 Missing :warning:
echopype/core.py 66.66% 1 Missing :warning:
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1323       +/-   ##
===========================================
- Coverage   83.52%   48.48%   -35.04%     
===========================================
  Files          64       69        +5     
  Lines        5686     6472      +786     
===========================================
- Hits         4749     3138     -1611     
- Misses        937     3334     +2397     
Flag Coverage Δ
unittests 48.48% <87.46%> (-35.04%) :arrow_down:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

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

codecov-commenter avatar Jun 07 '24 23:06 codecov-commenter

Hey @dash-uvic : Thanks for fixing the pre-commits.

I've just approved for the CI to run on this PR. If you could add file conversion tests to run on some example test files, that will allow us to review the changes more easily.

The way we organized test data now is for you to share the files with us with the specific path you would like those to be in under echopype/test_data/ (so these should correspond with what your tests use). We will add them to a google drive that the CI pulls automatically on each test run. This will change hopefully later this summer/fall to use GitHub Release Assets, but for now we will continue with this setup.

leewujung avatar Jun 11 '24 05:06 leewujung

@dash-uvic : I saw that you added some tests and just "approved" the workflow to run - I think usually for a PR it just needs to be approved once and then it'll always run for new changes. If you see that stalled somehow feel free to ping me directly.

leewujung avatar Jun 29 '24 16:06 leewujung

@leewujung What's the best method for adding/sending you the test files (average ~50M but there is a larger ~400M file)?

dash-uvic avatar Jul 02 '24 15:07 dash-uvic

@dash-uvic : Is there a reason why the files need to be that large? If not, could you generated some smaller files? Large files will make CI runs a lot slower.

leewujung avatar Jul 02 '24 16:07 leewujung

@leewujung Yeah I can probably truncate the larger azfp file down to fewer timesteps and remove unrequired fields from the expected output Mat file. I think doing that I can get them down to ~10M if that's sufficient.

dash-uvic avatar Jul 02 '24 18:07 dash-uvic

@dash-uvic : Yes ~10MB would be nice. Thanks!

Also if it is easy for you to put the files on a cloud folder somewhere, I can download them and put them in the docker image we currently use for the CI. We would like to move all files to use the github release assets, but don't have time to do it just yet...

leewujung avatar Jul 02 '24 18:07 leewujung

@leewujung Ok sounds goods, I'll put them on a cloud and email you the link next week.

dash-uvic avatar Jul 04 '24 16:07 dash-uvic

@dash-uvic : Thanks for sending the test files. I added them to the CI. Looks like the tests all pass now!

Could you please fix that pre-commit.ci failure?

Should we merge this afterwards and call it alpha, or do you have additional changes you would like to add?

leewujung avatar Jul 12 '24 01:07 leewujung

@leewujung The pre-commit error has been fixed. Strangely, it did not pop up when I ran pre-commit locally. You can merge this as alpha. I will likely have changes in the future but that won't be for a while and can be a new PR.

dash-uvic avatar Jul 15 '24 15:07 dash-uvic

Great, thanks @dash-uvic! I wonder if that may be because of different versions of the pre-commit hooks.

I'll merge this now and add azfp6 alpha in the documentation. We will release v0.9.0 soon and this will be included in there. :)

leewujung avatar Jul 16 '24 14:07 leewujung