Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move preprocess functions and tests new clean and commongrid subpackages #993

Merged
merged 14 commits into from
Mar 21, 2023

Conversation

emiliom
Copy link
Collaborator

@emiliom emiliom commented Mar 16, 2023

Moved noise functions (public API: estimate_noise and remove_noise) from preprocess to filter. Split off noise tests in preprocess/test_preprocess.py to filter/test_noise.py

Fully addresses #889, moving functions from preprocess to filter or commongrid. We agreed to call the new subpackage commongrid rather than unify (I'll make a note of it in #889). Updated all tests.

See #817 (comment), but note that I did not carry out the filter.noise hierarchy proposed there. Looking through all subpackages, there are only a couple of cases of another subpackage (folder) under a subpackage, and those are not publicly exposed.

NOTE: We'll likely need to revisit the subpackage name filter, because filter is a Python reserved word!

Partly addresses #889. This PR could be treated as stand-alone, with a follow up PR to address the rest of #889. Or we could add those other changes on top of this PR, when ready.

@emiliom emiliom added the design label Mar 16, 2023
@emiliom emiliom added this to the 0.7.0 milestone Mar 16, 2023
@emiliom emiliom requested a review from leewujung March 16, 2023 22:59
@emiliom emiliom self-assigned this Mar 16, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 16, 2023

Codecov Report

Merging #993 (295111b) into dev (edc1d15) will decrease coverage by 55.71%.
The diff coverage is 82.92%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff             @@
##              dev     #993       +/-   ##
===========================================
- Coverage   79.72%   24.01%   -55.71%     
===========================================
  Files          62       66        +4     
  Lines        5721     5738       +17     
===========================================
- Hits         4561     1378     -3183     
- Misses       1160     4360     +3200     
Flag Coverage Δ
unittests 24.01% <82.92%> (-55.71%) ⬇️

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

Impacted Files Coverage Δ
echopype/clean/noise_est.py 94.73% <ø> (ø)
echopype/commongrid/mvbs.py 84.37% <ø> (ø)
echopype/preprocess/api.py 47.05% <47.05%> (-44.17%) ⬇️
echopype/clean/api.py 78.57% <78.57%> (ø)
echopype/commongrid/api.py 95.45% <95.45%> (ø)
echopype/__init__.py 100.00% <100.00%> (ø)
echopype/clean/__init__.py 100.00% <100.00%> (ø)
echopype/commongrid/__init__.py 100.00% <100.00%> (ø)
echopype/consolidate/api.py 91.78% <100.00%> (ø)
echopype/preprocess/__init__.py 100.00% <100.00%> (ø)

... and 43 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@leewujung
Copy link
Member

Partly addresses #889. This PR could be treated as stand-alone, with a follow up PR to address the rest of #889. Or we could add those other changes on top of this PR, when ready.

Hey @emiliom : thanks for submitting this! I think we could keep this PR open so that you could add the rest of #889 on top of this? I can review this PR again at that time. I did look through the changes in this one quickly and your proposed changes to where estimate_noise sits is fine by me.

echopype/filter/api.py Outdated Show resolved Hide resolved
@emiliom emiliom changed the title Move noise functions and tests from preprocess to new filter subpackage Move preprocess functions and tests new filter and commongrid subpackages Mar 17, 2023
@emiliom
Copy link
Collaborator Author

emiliom commented Mar 17, 2023

Alright, it took me much more work than I anticipated to complete the rest of this PR, but here it is.

  • All preprocess functions have now been distributed to either filter or commongrid
  • All tests were updated and renamed as needed
  • .ci_helpers/run-test.py is updated
  • Added stub, pass-through functions to preprocess with a deprecation warning saying that they'll be removed in 0.7.1. I didn't add tests for these stub functions, since they seem unnecessary. I did test them locally, manually.
  • Updated the provenance attribute statements to replace "preprocess" with "filter" or "commongrid". Note that I didn't make other changes to that code, since we're having a parallel conversation about the future of those attributes.

All tests ran locally.

@emiliom
Copy link
Collaborator Author

emiliom commented Mar 20, 2023

New commit to change filter subpackage to clean. See #889 (comment)

@emiliom emiliom changed the title Move preprocess functions and tests new filter and commongrid subpackages Move preprocess functions and tests new clean and commongrid subpackages Mar 21, 2023
Copy link
Member

@leewujung leewujung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emiliom : Thanks for this PR! I think this is ready to be merged once you address the estimate_noise docstring change (which I know didn't come from you...). My other comments were just questions which may or may not lead to further changes.

.ci_helpers/run-test.py Outdated Show resolved Hide resolved
echopype/clean/api.py Outdated Show resolved Hide resolved
echopype/commongrid/api.py Show resolved Hide resolved
echopype/__init__.py Outdated Show resolved Hide resolved
.ci_helpers/run-test.py Outdated Show resolved Hide resolved
@emiliom
Copy link
Collaborator Author

emiliom commented Mar 21, 2023

I think the only open issue is whether to reorder the __all__ strings in echopype/__init__.py. Any preference?

@leewujung
Copy link
Member

whether to reorder the __all__ strings in echopype/__init__.py. Any preference?

I think it will be good to reorder that as well! :)

echopype/__init__.py Outdated Show resolved Hide resolved
Sorting __all__ strings in alphabetical order
@emiliom
Copy link
Collaborator Author

emiliom commented Mar 21, 2023

whether to reorder the all strings in echopype/init.py. Any preference?

I think it will be good to reorder that as well! :)

Done. Everything else is also taken care of. I'll merge the PR once the tests are completed. Thanks!!

@emiliom emiliom merged commit b802e8c into OSOceanAcoustics:dev Mar 21, 2023
@emiliom emiliom deleted the preprocess-reorg branch March 21, 2023 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants