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

Allow spectral filtering of input data #137

Merged

Conversation

JuliaKukulies
Copy link
Member

This pull request adds the spectral filtering feature (that has already been added to v2.0-dev though #65) to dev. The main changes I have made to the code are

  • to add the function spectral_filtering() in utils.py
  • small changes in feature_detection.py to implement the filter step within the feature detection (same as for the optional gaussian filter)
  • to add tests for the spectral_filtering() function itself and the filtering option in the feature detection function

For better comprehension what the function does, you can also check out the example notebook

@freemansw1 freemansw1 added this to the Version 1.5 milestone Jun 10, 2022
@freemansw1 freemansw1 added the enhancement Addition of new features, or improved functionality of existing features label Jun 21, 2022
@freemansw1
Copy link
Member

Thanks @JuliaKukulies! I wonder if we should target a v1.4 release with these changes (and #138), given that the functionality is already in the v2.x branch and it seems to be well tested and less of a big change than #127 . I'm not sure. Either way, I'll try to get this reviewed next week.

@JuliaKukulies
Copy link
Member Author

@freemansw1 Yes, I totally think we could that and have an intermediate release with this and #138. You are right that the changes are rather small and that it has already been reviewed for v.2 (just note that I have added a unit test that was not there before and that I made small changes in existing functions, so could be good to pay extra attention to that one more time).

What about #136 then? It is quite a lot of new code and a comprehensive new feature, so maybe rather something for 1.5 together with #127, but on the other hand it is also easier to review given that it is just an addition of code rather than changes in existing code. So could also be ready for 1.4 already, if we go for a 1.4 release.

@freemansw1 freemansw1 modified the milestones: Version 1.5, Version 1.4 Jun 22, 2022
@freemansw1
Copy link
Member

@JuliaKukulies Agreed. Let's plan on a 1.4 release with these changes, mergers and splits, and documentation improvements. I've added new 1.3.2 and 1.4 milestones, and have (I hope) moved the appropriate PRs to the appropriate milestones.

@JuliaKukulies
Copy link
Member Author

Perfect, thanks @freemansw1 !

Copy link
Member

@freemansw1 freemansw1 left a comment

Choose a reason for hiding this comment

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

I just have a few comments, but I'm happy to approve/merge when we start staging for v1.4.

tobac/feature_detection.py Outdated Show resolved Hide resolved
tobac/tests/test_utils.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 29, 2022

Codecov Report

Merging #137 (7a27401) into RC_v1.4.0 (6e73fca) will increase coverage by 0.66%.
The diff coverage is 68.42%.

@@              Coverage Diff              @@
##           RC_v1.4.0     #137      +/-   ##
=============================================
+ Coverage      32.09%   32.75%   +0.66%     
=============================================
  Files             11       11              
  Lines           2047     2085      +38     
=============================================
+ Hits             657      683      +26     
- Misses          1390     1402      +12     
Flag Coverage Δ
unittests 32.75% <68.42%> (+0.66%) ⬆️

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

Impacted Files Coverage Δ
tobac/feature_detection.py 70.16% <26.66%> (-3.94%) ⬇️
tobac/utils.py 46.38% <95.65%> (+5.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e73fca...7a27401. Read the comment docs.

@JuliaKukulies
Copy link
Member Author

JuliaKukulies commented Jun 29, 2022

@freemansw1 I think I have addressed your two remaining points now. Sorry that this resulted in so many commits. I had some issues with the tests, as I had to change the unit at more locations than I thought. But now it should be fine. The main difference to the original PR are

  • changed required wavelength units to m instead of km (this included also code changes where dxy was locally changed to get the same unit as the wavelengths, so actually the code should be cleaner now)
  • modified test function for spectral_filtering() on data that actually has a wave signal rather than only any random data

@freemansw1 freemansw1 self-requested a review June 30, 2022 02:50
Copy link
Member

@freemansw1 freemansw1 left a comment

Choose a reason for hiding this comment

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

I'm happy with these changes now! We should wait to merge until we are staging for v1.4.0, but I am happy to merge otherwise.

@JuliaKukulies
Copy link
Member Author

Great, thanks for your review! Yes, we wait for v1.4.0

@freemansw1 freemansw1 changed the base branch from dev to RC_v1.4.0 July 9, 2022 16:38
@freemansw1
Copy link
Member

I've moved this to merge into the new RC_v1.4.0 now, so I'm happy for you to merge when ready @JuliaKukulies!

@JuliaKukulies
Copy link
Member Author

All right, thanks @freemansw1! @w-k-jones do you think you wanna have a look as well or should I go ahead and merge?

@freemansw1
Copy link
Member

Note with the merge of #138, auto-merging now fails with the documentation being at issue.

@JuliaKukulies JuliaKukulies merged commit 3d736d6 into tobac-project:RC_v1.4.0 Jul 23, 2022
@JuliaKukulies
Copy link
Member Author

I went ahead and merged this now after solving the merge conflict with #138, since you approved @freemansw1. I hope that was OK!

@JuliaKukulies JuliaKukulies deleted the spectral_filtering branch February 22, 2023 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Addition of new features, or improved functionality of existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants