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

Remove automatic version conversion in `open_converted` #1143

Closed leewujung closed 1 year ago

leewujung commented 1 year ago

Overview

This PR removes the automatic EchoData format conversion when using open_converted on already converted data from earlier versions of echopype.

This PR does not remove the previous data format conversion mechanisms (under echodata/sensor_ep_version_mapping) in case we are able to reinstate this capability in the future.

The documentation is also updated.

Context

We included a converter to automatically convert v0.5.x EchoData format to v0.6.x EchoData format. However, we have not been able to create such a functionality for all later versions, including the latest v0.8.0 which included a large number of data format changes. The existence of automatic v0.5.x to v0.6.x format conversion becomes confusing in this case since no other versions of data are able to be converted automatically in the same way, and using open_raw to re-convert the data is necessary.

This is discovered by @emiliom

we have a problem with 0.8.0. I was trying to run and update the echopype-examples notebooks, and ran into a failure with open_converted that boils down to https://github.com/OSOceanAcoustics/echopype/blob/254ee106eb580959ca85f2131168c4f17c0a285a/echopype/echodata/sensor_ep_version_mapping/ep_version_mapper.py#L22, called by https://github.com/OSOceanAcoustics/echopype/blob/254ee106eb580959ca85f2131168c4f17c0a285a/echopype/echodata/echodata.py#L165. ie, the version tag 0.8.0 raises an error. We didn't run into this during local tests b/c the version tag was still 0.7.2xxx. But I'm puzzled why it didn't come up in the CI once the version was stamped as 0.8.0.

review-notebook-app[bot] commented 1 year ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

codecov-commenter commented 1 year ago

Codecov Report

Merging #1143 (d5f9326) into dev (31b0665) will decrease coverage by 0.05%. Report is 5 commits behind head on dev. The diff coverage is n/a.

@@            Coverage Diff             @@
##              dev    #1143      +/-   ##
==========================================
- Coverage   79.78%   79.74%   -0.05%     
==========================================
  Files          65       10      -55     
  Lines        6011      711    -5300     
==========================================
- Hits         4796      567    -4229     
+ Misses       1215      144    -1071     
Flag Coverage Δ
unittests 79.74% <ø> (-0.05%) :arrow_down:

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

Files Changed Coverage Δ
echopype/echodata/echodata.py 76.92% <ø> (-5.14%) :arrow_down:

... and 56 files with indirect coverage changes

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

leewujung commented 1 year ago

@emiliom : Looking at the tests, if we take the approach to just removing the auto-conversion under the hood of open_converted, we can just remove the entire tests/echodata/test_echodata_structure.py altogether. What do you think? These test have not been run for a long time now anyway...

emiliom commented 1 year ago

This PR removes the automatic EchoData format conversion when using open_converted on already converted data from earlier versions of echopype.

This PR does not remove the previous data format conversion mechanisms (under echodata/sensor_ep_version_mapping) in case we are able to reinstate this capability in the future.

I agree with that approach. Thanks!

@emiliom : Looking at the tests, if we take the approach to just removing the auto-conversion under the hood of open_converted, we can just remove the entire tests/echodata/test_echodata_structure.py altogether. What do you think? These test have not been run for a long time now anyway...

You're right, the only actual test in there was set to xfail quite a while ago. But I'm torn about whether to remove the test module file altogether ...

leewujung commented 1 year ago

I can't decide if I'd rather keep test_echodata_structure.py even though it's not doing anything. You decide!

Haha, ok. I decided that we'll keep it under a different filename (test_echodata_version_convert, instead of test_echodata_structure), and I've added notes on the reason behind this decision -- I think these may be useful to keep as reference, just like how we are keeping echodata/sensor_ep_version_mapping.py.

I'll merge this now!