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

Revamp tests after CI upgrade and API change #278

Closed leewujung closed 3 years ago

leewujung commented 3 years ago

With the new CI running we currently have tests for converting files with different combinations of sources/destinations, but we still need to revamp the old tests that actually test for the data values in the converted files and the calibration results.

We need these incorporated in the CI and also update the tests with the new API calls.

In the transition time from the old to the new API calls, we also need tests that used the old syntax to make sure the potential breaking changes are taken care of.

Here's a list of tests to be revamped:

Below we'll defer to the next release:

Updates Apr 28

There are currently unresolved issues related to matching echopype pulse compressed Sv with those generated by Matlab code and EchoView. The Matlab and Echoview pulse compression outputs are also different. This needs further investigation and should be its own issue.

Updates May 5

The EK80 CW angle conversion issues have been investigated in #319 and discrepancy noted in comments in the test.

leewujung commented 3 years ago

This is related #263 .

leewujung commented 3 years ago

@lsetiawan : for these tests there are additional test files that need to be included. Right now they are in my local echopype/test_data/xxxx folders but I can't push these files up due to LFS over quota problem. The test files were generated by other software like EchoView or Matlab code that we are testing echopype outputs against.

Could you help with incorporating these into the CI? We need to do this for some other tests too for calibrate, so maybe we work out a strategy between the two of us and @ngkavin to get things into the right place.

From #263 it seems that some, if not all, of the files are already in the Google drive. But it'll be good to have specific folder structure (I just made them) so that we know which ones go with which test and example data files. Specifically they are:

Note: unfortunately for EK80 tests the files we test echopype outputs against are rather large (10s of MB for each frequency channel, for example). As we discussed, in a couple months we can make new files to solve the large file problem once and for all, but we need to get by at the moment...

leewujung commented 3 years ago

@lsetiawan @emiliom : Is there a way we can set up the CI such that some tests are only run when a certain part of the code is changed? That seems to be a straightforward way to reduce CI running time. For example the tests that check echopype convert outputs against those from other software only needs to be run if anything changed under convert. The tests that check calibration outputs only needs to be run if anything changed under calibrate. So on and so forth.

leewujung commented 3 years ago

275 includes most of the calibration tests, though notably the EK80 calibration results need further testing, since I am finding some difference with both the EchoView and Matlab outputs.

279 includes most of the convert tests.

leewujung commented 3 years ago

295 includes revamped noise removal and MVBS computation.

leewujung commented 3 years ago

@lsetiawan @emiliom : Is there a way we can set up the CI such that some tests are only run when a certain part of the code is changed? That seems to be a straightforward way to reduce CI running time. For example the tests that check echopype convert outputs against those from other software only needs to be run if anything changed under convert. The tests that check calibration outputs only needs to be run if anything changed under calibrate. So on and so forth.

This is now added via #280 .

leewujung commented 3 years ago

305 addresses potential issue in XML parsing that is partially related to the different models of EK80.

leewujung commented 3 years ago

319 addresses the EK80 angle conversion issue.

leewujung commented 3 years ago

With the majority of tests done and the remaining ones captured in separate issues, we are ready to close this.