OSOceanAcoustics / echopype

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

Refactor XML parser in `parse_azfp.py` to maintain consistency with other parsers #1135

Closed praneethratna closed 12 months ago

praneethratna commented 1 year ago

Fixes #659. Now parse_azfp.py uses xml.etree.ElementTree instead of xm.dom.minidom for parsing.

Update: More parameters are being parsed now, and some of those were added to the EchoData object via set_groups.

CC @leewujung @emiliom

codecov-commenter commented 1 year ago

Codecov Report

Merging #1135 (1aaa73f) into dev (3ca028f) will decrease coverage by 16.64%. The diff coverage is 88.63%.

@@             Coverage Diff             @@
##              dev    #1135       +/-   ##
===========================================
- Coverage   82.35%   65.71%   -16.64%     
===========================================
  Files          64       35       -29     
  Lines        5803     4384     -1419     
===========================================
- Hits         4779     2881     -1898     
- Misses       1024     1503      +479     
Flag Coverage Δ
unittests 65.71% <88.63%> (-16.64%) :arrow_down:

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

Files Changed Coverage Δ
echopype/calibrate/api.py 85.41% <ø> (ø)
echopype/convert/utils/ek_raw_parsers.py 39.73% <66.66%> (-16.18%) :arrow_down:
echopype/utils/misc.py 85.71% <85.71%> (ø)
echopype/convert/parse_azfp.py 77.48% <89.65%> (-15.49%) :arrow_down:
echopype/calibrate/cal_params.py 91.79% <100.00%> (ø)
echopype/calibrate/calibrate_azfp.py 94.87% <100.00%> (ø)
echopype/calibrate/range.py 87.69% <100.00%> (ø)
echopype/convert/set_groups_azfp.py 78.16% <100.00%> (-19.52%) :arrow_down:

... and 50 files with indirect coverage changes

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

praneethratna commented 12 months ago

Closing and re opening the PR to re-run the tests.

praneethratna commented 12 months ago

I opened up a new issue for additional work in set_groups_azfp to store parameters that are potentially varying across the "phases": #1171 Let me know if you'd like to contribute to that!

Sure, would love to contribute to #1171 once this is merged.

leewujung commented 12 months ago

Thanks a lot @praneethratna! I did a direct push for some cosmetic changes which you can see above, and will go ahead to merge this once the tests run through. Excited to have this upgrade in!

emiliom commented 12 months ago

@praneethratna see the new test errors. They look like they'll be easy to fix.

praneethratna commented 12 months ago

@praneethratna see the new test errors. They look like they'll be easy to fix.

I actually thought about this error but forgot while merging😅. I've fixed the error now and pushed the changes!

praneethratna commented 12 months ago

@emiliom I'm sorry for all these messed up commits, kept forgetting minor changes in code. I think this can be merged now as the failing test is not related this PR.

emiliom commented 12 months ago

@emiliom I'm sorry for all these messed up commits, kept forgetting minor changes in code. I think this can be merged now as the failing test is not related this PR.

Can you elaborate? I realize it's only happening with Python 3.11, and I can see that it looks like the same TypeError in echopype/tests/convert/test_parsed_to_zarr.py: "TypeError: 'numpy.float64' object cannot be interpreted as an integer". But if it wasn't happening before this PR, it seems like it's linked to this PR. Do you remember if it's been happening with 3.11 since you first created the PR?

Are you set up to run tests locally? If not, I can try running test_parsed_to_zarr.py locally in a 3.11 conda environment.

praneethratna commented 12 months ago

Can you elaborate? I realize it's only happening with Python 3.11, and I can see that it looks like the same TypeError in echopype/tests/convert/test_parsed_to_zarr.py: "TypeError: 'numpy.float64' object cannot be interpreted as an integer". But if it wasn't happening before this PR, it seems like it's linked to this PR. Do you remember if it's been happening with 3.11 since you first created the PR?

Yes, it's been happening since i first created this PR.

Are you set up to run tests locally? If not, I can try running test_parsed_to_zarr.py locally in a 3.11 conda environment.

There seems to be an issue with setting up conda in my laptop, so I don't use any conda environment while running the tests.

emiliom commented 12 months ago

Yes, it's been happening since i first created this PR.

Hmm, that's odd. I'll take a look locally. In the meantime, how about if you start planning for #1171? Thanks.

emiliom commented 12 months ago

All the tests in echopype/tests/convert/test_parsed_to_zarr.py run fine locally with Python 3.11, with a freshly created conda environment, on both dev and an older version of this PR. So, I don't know what's going on, but it looks like it's limited to the CI.

I'll take a final look at the PR to confirm that everything has been addressed.