-
Notifications
You must be signed in to change notification settings - Fork 65
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
[ENH] - Tech Audit #2: Accuracy checks for Hilbert Transform, Filters, and Spectra #204
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.
These tests updates look good to me!
Okay, slightly unconventional review approach here. I thought I had just some small updates, so I started doing directly. Then I realized I had a bit more to suggest - which I thought I would add as comments, rather than digging in too deep with direct edits. So, in the end I did pushed an update commit, and I'm also leaving some review comments. Please take a look at my direct commit, to sanity check, and get a sense of what I was going for. Basically, that commit is trying to consolidate on using general test settings and refactoring some code a bit. |
@elybrand - thanks for the updates here! These look much better / more specific tests than we had before. I've made some comments / suggestions. A bunch of these things are more code mechanics, so if you prefer you can tag in @ryanhammonds to work on the code and organizational elements here, and we can coordinate this one as a team effort. I also wasn't totally sure if this PR was done, or a work-in-progress (if you plan on adding more test updates). Note that if things are in progress, you can use the tag |
…ription for testing instantaneous phase.
This looks great @elybrand! I've made some small comments for minor organizational things. Once you get a chance to check and update those, then I think this should be all good! |
…fixture for testing spectral power methods.
Looks good to me, merging in! |
This PR adds accuracy checks (#194) for
compute_spectrum
with themedfilt
andwelch
options. It uses a low frequency sinusoid to compare against where the power spectrum should be a dirac spike at frequency 1 and zero elsewhere.