Dev branch test failures need to be fixed

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

Dev branch test failures need to be fixed #553

Closed lsetiawan closed 2 years ago

lsetiawan commented 2 years ago

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:

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

leewujung commented 2 years ago

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 commented 2 years ago

I haven't looked into this, but I am pretty sure that it is related to changes in https://github.com/OSOceanAcoustics/echopype/pull/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 (https://github.com/OSOceanAcoustics/echopype/issues/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 commented 2 years ago

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 commented 2 years ago

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!