OSOceanAcoustics / echopype

Enabling interoperability and scalability in ocean sonar data analysis
https://echopype.readthedocs.io/
Apache License 2.0
94 stars 73 forks source link

added support for AZFP multiple phase attributes parsing #1182

Closed praneethratna closed 10 months ago

praneethratna commented 11 months ago

Addresses #1171 and now XML parameters from multiple phases are stored in the Vendor_Specific group.

codecov-commenter commented 11 months ago

Codecov Report

Merging #1182 (7cab189) into dev (ea01132) will decrease coverage by 5.28%. Report is 3 commits behind head on dev. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev    #1182      +/-   ##
==========================================
- Coverage   82.37%   77.09%   -5.28%     
==========================================
  Files          65       19      -46     
  Lines        5810     3061    -2749     
==========================================
- Hits         4786     2360    -2426     
+ Misses       1024      701     -323     
Flag Coverage Δ
unittests 77.09% <100.00%> (-5.28%) :arrow_down:

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

Files Coverage Δ
echopype/convert/parse_azfp.py 93.53% <100.00%> (+0.33%) :arrow_up:
echopype/convert/set_groups_azfp.py 97.95% <100.00%> (+0.25%) :arrow_up:

... and 56 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

emiliom commented 11 months ago

Hmm, now some tests are failing :cry: . @praneethratna can you look into the error messages to see what's going on?

praneethratna commented 11 months ago

Hmm, now some tests are failing 😢 . @praneethratna can you look into the error messages to see what's going on?

Yeah sure, but they doesn't seem to be related to my changes, but anyway closing and reopening the PR so that tests will run again.

emiliom commented 11 months ago

Yeah sure, but they doesn't seem to be related to my changes, but anyway closing and reopening the PR so that tests will run again.

Ah. Thanks for rerunning the tests. They're still failing. I'll try to take a closer look tomorrow. Your changes impact how AZFP files are processed; so, depending on which tests are failing, and on what data files, it is possible that the changes are causing this. But it sounds like you've already looked closely and discarded that possibility?

praneethratna commented 11 months ago

Yeah sure, but they doesn't seem to be related to my changes, but anyway closing and reopening the PR so that tests will run again.

Ah. Thanks for rerunning the tests. They're still failing. I'll try to take a closer look tomorrow. Your changes impact how AZFP files are processed; so, depending on which tests are failing, and on what data files, it is possible that the changes are causing this. But it sounds like you've already looked closely and discarded that possibility?

Yes, I've did a tear-down of docker containers and re-deployed containers on my laptop and the tests are running fine. I'm not sure why they are failing here.

emiliom commented 11 months ago

Ah, thanks! I didn't know you were now set up to run the test suite locally. Fantastic!

praneethratna commented 10 months ago

Ah, thanks! I didn't know you were now set up to run the test suite locally. Fantastic!

Ah, yes! I did set up the test suite locally during the first PR itself but I've had problems with conda on my laptop. So I can basically run all the tests in python 3.10 only.

emiliom commented 10 months ago

FYI I ran the tests locally with Python 3.10. There were 3 errors. Yeah, they do look to be unrelated to this PR. All 3 errors involve echopype/tests/convert/test_parsed_to_zarr.py::TestParsed2Zarr::test__create_zarr_info.

I'll follow up with that elsewhere. We may end up merging this PR before we resolve those other problems, but for now I'd like to investigate further.

emiliom commented 10 months ago

I am persuaded that this PR is fine and can be merged, and the CI errors are cause by an unrelated problem. So, I'll go ahead and merge it now. Thanks @praneethratna !!

I'll open a new issue to address the CI problem.