-
Notifications
You must be signed in to change notification settings - Fork 76
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
Conversation
Codecov Report
📣 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
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 43 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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 |
…references to preprocess
…a deprecation warning
preprocess
to new filter
subpackagepreprocess
functions and tests new filter
and commongrid
subpackages
for more information, see https://pre-commit.ci
Alright, it took me much more work than I anticipated to complete the rest of this PR, but here it is.
All tests ran locally. |
New commit to change |
preprocess
functions and tests new filter
and commongrid
subpackagespreprocess
functions and tests new clean
and commongrid
subpackages
There was a problem hiding this 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.
Co-authored-by: Wu-Jung Lee <[email protected]>
I think the only open issue is whether to reorder the |
I think it will be good to reorder that as well! :) |
Co-authored-by: Wu-Jung Lee <[email protected]>
Sorting __all__ strings in alphabetical order
Done. Everything else is also taken care of. I'll merge the PR once the tests are completed. Thanks!! |
Moved noise functions (public API:estimate_noise
andremove_noise
) frompreprocess
tofilter
. Split off noise tests inpreprocess/test_preprocess.py
tofilter/test_noise.py
Fully addresses #889, moving functions from
preprocess
tofilter
orcommongrid
. We agreed to call the new subpackagecommongrid
rather thanunify
(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
, becausefilter
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.