Open ctuguinay opened 1 month ago
Attention: Patch coverage is 95.65217%
with 4 lines
in your changes missing coverage. Please review.
Project coverage is 97.97%. Comparing base (
9f56124
) to head (b8f8ece
). Report is 119 commits behind head on main.
Files | Patch % | Lines |
---|---|---|
echopype/clean/api.py | 90.24% | 4 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
I wasn't sure if the transient noise bins indicated in the Ryan 2015 paper were radius or diameter values, but I couldn't run some examples if my assumption was radius (memory leakage problem since bins were so big) so I modified my assumption to diameter, and that is reflected in the default values of the transient noise mask function.
@leewujung This PR is at a good spot for review
For
mask_transient_noise
: Why do we need to copy operations in the functions?# Copy `ds_Sv` ds_Sv = ds_Sv.copy()
This copy is then copied again in
index_binning_pool_Sv
so I also commented there. I may be missing something, but I think we just need 1 copy that gets sliced with to exclude the shallower samples (exclude_above
).
I agree it's not necessary. Even that second copy isn't required since when I remove all of them, the tests seem to pass.
@leewujung Thanks for the review! This should be ready to look at again. I applied most of your suggestions, but I have a few questions/comments.
Addresses #1321