-
Notifications
You must be signed in to change notification settings - Fork 100
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
[DOC] - Add a line noise example #203
Conversation
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.
Looks good! I left a few comments/questions. I'm also about to push an accuracy test. Feel free to drop the test change if you think it's too much.
# Get the set of frequency values that need to be interpolated | ||
interp_mask = np.logical_and(freqs >= interp_range[0], freqs <= interp_range[1]) | ||
interp_freqs = freqs[interp_mask] | ||
# If given a list of interpolation zones, recurse to apply each one |
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.
After working on the #198, do you think it would be useful to generalize interpolate_spectrum
to interpolate_spectra
? I could see a case where one would want to remove line noise systematically across multiple spectra. Looping this function would be easy enough, however.
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.
This is a great idea! Just like the other util function, trim_spectrum
, does, interpolate_spectrum
(or renamed to spectra
should totally support 2D arrays. If there is line noise in a dataset, it should be consistent across channels, and so applying this to a 2D collection of PSDs is if anything likely to be more common than to 1D.
Do you want to try out generalizing the interpolation to 2D? It can go into a new PR I think.
Thanks for the review @ryanhammonds - and the accuracy checks look great! I made some comments on specific points - I don't think there is anything needs doing to this PR now, so I'll merge, but a PR for extending interpolation would be great :). |
A common question is how to deal with line noise, and so I started collecting my responses to turn into a proper example, which I finally fixed up and is added here.
In the process, I ended up updating
interpolate_spectrum
a bit to support multiple interpolation zones.I think everything here should be relatively straight-forward - @ryanhammonds, it would be great if you could read through the example for wording quirks, and check that the code updates make sense to you. Thanks!