Set range entries with no backscatter data to NaN in output of echodata.compute_range()

[leewujung]

This PR sets entries in the range data variable in the output dataset from compute_Sv that does not have corresponding backscatter entries (also set to NaN) to NaN. Some backscatter entries are NaN due to changes in echosounder settings, such as data collection range, transmit signal duration, etc.

The change addresses the potential confusion #483.

Tasks

• set range entries with no backscatter data to NaN in echodata.compute_range()
• update tests to test for NaN entries

[codecov-commenter]

Codecov Report

Merging #547 (e5ae984) into dev (3061e0b) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev     #547      +/-   ##
==========================================
- Coverage   78.09%   78.07%   -0.02%     
==========================================
  Files          40       10      -30     
  Lines        3428      447    -2981     
==========================================
- Hits         2677      349    -2328     
+ Misses        751       98     -653     

Flag	Coverage Δ
unittests	78.07% <100.00%> (-0.02%)	⬇️

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

Impacted Files	Coverage Δ
echopype/echodata/echodata.py	83.18% <100.00%> (-7.12%)	⬇️
echopype/preprocess/api.py	66.66% <100.00%> (-20.52%)	⬇️
echopype/__init__.py	0.00% <0.00%> (-100.00%)	⬇️
echopype/preprocess/__init__.py	0.00% <0.00%> (-100.00%)	⬇️
echopype/preprocess/noise_est.py	71.42% <0.00%> (-22.86%)	⬇️
echopype/utils/repr.py
echopype/convert/convert.py
echopype/calibrate/api.py
... and 27 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3061e0b...e5ae984. Read the comment docs.

[leewujung]

@lsetiawan : In our current testing setup, since this PR only "touches" echodata.py, only tests under tests/echodata were run. In practice, the method (compute_range()) being updated here is used in the calibration functions, so tests under tests/calibrate should also be run. Right now the entire set of tests are only run when merged to main so the calibration tests will only be run then.

What's the best practice here? I think we were tripped a few times by this (i.e., additional fixes needed when merging dev to main due to the adaptive test not catching bugs that are downstream from the code being modified in the PR).

Can we set up something that pulls out the downstream pieces that uses the changed functions and run the corresponding tests? If we do this, at what stage this should be run -- when it is merged to dev, or as a label such as what is done with putting on the label Needs Complete Testing?

[leewujung]

@lsetiawan : I merged your #523 and used the fixture structure for the test here. Also added an EK60 file (test_data/ek60/Winter2017-D20170115-T150122.raw) that contains NaN entries in addition to the EK80 file (test_data/ek80/D20170912-T234910.raw) that we already have there.

[lsetiawan]

What's the best practice here? I think we were tripped a few times by this (i.e., additional fixes needed when merging dev to main due to the adaptive test not catching bugs that are downstream from the code being modified in the PR).

The CI is now setup to run all tests when merged to dev as shown here:

echopype/.github/workflows/ci.yaml

Lines 91 to 94 in 0f699cc

	- name: Running All Tests
	shell: bash -l {0}
	run: |
	pytest -vv -rx --cov=echopype --cov-report=xml --log-cli-level=WARNING --disable-warnings |& tee ci_${{ matrix.python-version }}_test_log.log

So as a first step, as long as the isolated module test works you can merge to dev for a full test. Or you can always just use the Needs complete testing label with the PR if you know that particular function touches many different things. In practice, most software CI actually runs everything anyways to make sure your changes doesn't break anything, we just decided to make this custom, brittle, intelligence 😜

[emiliom]

Or you can always just use the Needs complete testing label with the PR if you know that particular function touches many different things.

And whenever you have a need for that mechanism but don't remember how to use it, go to the docs: https://echopype.readthedocs.io/en/stable/contributing.html#continuous-integration-github-actions

[leewujung]

Hmm, we should revisit this soon, in terms of how/what we want the tests to be set up. The past few times tests failed when merging to main I think was after the supposedly complete test when merging to dev, and we opted for fixing the bugs for obvious reasons then.

In theory we can get out of the current brittle setup once most big test data files are updated to a smaller size. I updated 2 major ones, but ran out of time to do the last couple files before I had to return a hardware dongle needed to generate data we test against -- the next time I can do this is likely late March - April.

BUT regardless, it is unlikely we'll be able to reduce all files to be KB size like the new EK80 test files I collected, since we don't have access to many instrument variations that were used to collect the diverse test files.

[lsetiawan]

LGTM! Thanks for this awesome work. I need to think about the whole testing scheme and see how we can improve it 😄 Would you mind, pasting all of the thoughts from here to an issue so we can discuss there? Thanks @leewujung

[lsetiawan]

The past few times tests failed when merging to main I think was after the supposedly complete test when merging to dev, and we opted for fixing the bugs for obvious reasons then.

Just checked this. The tests failed the past few times on main due to the publish to PyPi action issues, which is more of a packaging issue rather than code. I think I need to figure out how to activate that within the dev branch also so it imitates main. In terms of full CI testing PR #411 fixed the missing testing since the test for all testing are actually run directly with pytest rather than the small script I created.

[leewujung]

Just checked this. The tests failed the past few times on main due to the publish to PyPi action issues, which is more of a packaging issue rather than code.

Thanks for checking @lsetiawan , I must have confused that and the other earlier ones! Great that there was no unexpected behavior!

And yes I'll paste the thoughts we've had here to a new issue. :)