Add Index Aggregation Option for Cleaning Mask Functions #1326
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1326 +/- ##
===========================================
+ Coverage 83.52% 97.96% +14.44%
===========================================
Files 64 3 -61
Lines 5686 197 -5489
===========================================
- Hits 4749 193 -4556
+ Misses 937 4 -933
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
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 |
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
@leewujung This should be ready to review again. I added this reflect logic we talked about previously |
echopype/clean/api.py
Outdated
| upper_limit_sl : str, default `400m` | ||
| Upper limit of deep scattering layer line (m). | ||
| lower_limit_sl : Union[int, float], default `500`m | ||
| lower_limit_sl : str, default `500m` | ||
| Lower limit of deep scattering layer line (m). | ||
| num_side_pings : int, default `15` | ||
| Number of preceding & subsequent pings defining the block. | ||
| attenuation_signal_threshold : Union[int, float], default `8.0`dB | ||
| attenuation_signal_threshold : str, default `8.0dB` |
There was a problem hiding this comment.
Could you change ` here to ", and also elsewhere? It's just that people need to put in quotes and not backticks as input arguments.
There was a problem hiding this comment.
Made this change 👍
| "Decibal input must be a string formatted as `NUMdB` or `NUMdb." | ||
| f"Cannot be of type `{type(dB_str)}`." | ||
| ) | ||
| pattern = r"^[-+]?\d+\.?\d*(?:dB|db)$" # Ensures exact match with format |
There was a problem hiding this comment.
do you want to add an optional whitespace before dB/db so that inputs could be NUMdB or NUM dB?
There was a problem hiding this comment.
I don't like having this space here. Maybe we just keep it NUMdB without space? This way it's also simpler and the user only has 2 accepted ways to write it, NUMdB and NUMdb, instead of 4 ways to write it
There was a problem hiding this comment.
i think this is fine. I just remembered that we also only allow "10m".
echopype/clean/api.py
Outdated
| logger.warning( | ||
| "Consider using `func=nanmean`. `func=nanmedian` is an incredibly slow operation due " | ||
| "to the overhead sorting." | ||
| ) |
There was a problem hiding this comment.
I agree with the statement, but the original method used nanmedian right? I feel that we should not recommend the change, but we can warn people that it is SLOW. We can also point them to the issue for the Fielding filter and the echopy code for that in the warning?
There was a problem hiding this comment.
Added this. Good idea! 👍
echopype/clean/api.py
Outdated
| func: str, default "nanmean" | ||
| Pooling function used in the pooled Sv aggregation. |
There was a problem hiding this comment.
I feel we should default to "nanmedian" since that is what the original method said.
There was a problem hiding this comment.
Made this change
There was a problem hiding this comment.
@ctuguinay : Thanks for the changes. I had pretty minor comments, and think we can merge this afterwards. This is such an entrenched issue, and probably something the AA SI folks can enhance further later.
|
@leewujung Thanks for the review! This should be ready for a short review again |
Co-authored-by: Wu-Jung Lee <[email protected]>
Co-authored-by: Wu-Jung Lee <[email protected]>
|
@leewujung oh could you approve this again. i think committing your suggestions need further approval |
Addresses #1321