Dev branch test failures need to be fixed

[lsetiawan]

Overview

The dev branch is currently failing. Seems like the failures has to do something with the visualization module. This need to be fixed before v0.5.6 is released.

Tests to fix

Here are the list of tests that failed:

• echopype/tests/visualize/test_plot.py::test_plot_multi_get_range[filepath1-EK80-None-range_kwargs1] FAILED [ 94%]
• echopype/tests/visualize/test_plot.py::test_plot_Sv[filepath1-EK80-None-range_kwargs1] FAILED [ 95%]

[leewujung]

I haven't looked into this, but I am pretty sure that it is related to changes in #547, which replaces a subset of entries of the range variable to NaN.

Another case of how the downstream tests not being run automatically before merged to dev (#552)! Thought it is partially my fault to not have used the "Need complete testing" label!

[lsetiawan]

I haven't looked into this, but I am pretty sure that it is related to changes in #547, which replaces a subset of entries of the range variable to NaN.

I think so as well. Do you think you could help me with this? Thanks @leewujung 🙏🏽

Another case of how the downstream tests not being run automatically before merged to dev (#552)! Thought it is partially my fault to not have used the "Need complete testing" label!

Ahhhh... I didn't realize you didn't put this label on the PR. I merged too soon 😛 . But it's alright, dev really is just a staging area anyhow, we don't guarantee things to be working in the dev branch. 😄 Though main branch we do. This is the differences of the two.. I know sometimes I forget why we have main and dev.

[leewujung]

But it's alright, dev really is just a staging area anyhow, we don't guarantee things to be working in the dev branch. 😄 Though main branch we do. This is the differences of the two.. I know sometimes I forget why we have main and dev.

Heh, I definitely did not forget about the distinction. 😉 dev is a staging area, so having things completely flushed out there is important. I think it is less than ideal if PRs often fails when merged to dev, because that could result in multiple PRs addressing the same issue -- it's not a big deal, but the commit history would be harder to keep track of in the long run.

Using the "Need complete testing" is a way to address that, but since it is easy to forget, we should consider whether to revert back to run through all tests in the CI (re discussions in #552). We'll soon be adding more processing capabilities and having a robust mechanism to ensure that we are not operating in a "patching" mode would be critical.

[leewujung]

I haven't looked into this, but I am pretty sure that it is related to changes in #547, which replaces a subset of entries of the range variable to NaN.

I think so as well. Do you think you could help me with this? Thanks @leewujung 🙏🏽

Actually could you look into this first? I need to work on the other 2 PRs for v0.5.6 that those are substantial. I think this is related to how NaN is handled when plotting, if there's NaN entries in variables used as the x/y coordinates.

The NaN entries in the range data variable are where echodata.beam.backscatter_r data is NaN. If you plot the first and last ping of the test EK60 file along range_bin, or between the 2 frequencies in the EK80 test file you'll see what I mean. Thanks!