-
Notifications
You must be signed in to change notification settings - Fork 61
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
[BUG] - Technical Audit: Time Frequency edits #199
Conversation
…, and frequencies along the y-axis.
…orresponds to a fixed frequency.
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 looks good to me! For you second point though, it doesn't look like the test for compute_wavelet_transform
was changed. I can push tests updates after Tom reviews.
Okay, I had a look and reviewed the changes. I've made a commit - to fix / update one thing. The update for I have edited As well as checking the code, here's a sanity check: the old & new version applied to our documentation example. The new version changes the 'frequency slips'. Is this what you would expect the updates to do, and does this look good to you? If my updates look good to you Eric, and all this makes sense, then I think this is good to go, and can be merged. |
@TomDonoghue great catch with the nan's. Your commits look good to me. I'm not worried about the disappearance of the frequency "slips". Calling |
@elybrand - sounds good! With the example, I mostly wanted to check how it changed makes sense and isn't surprising to you (as in, nothing weird happened!). I trust your judgement on numerical stability, and agree that within the use cases / standards here, this is general practice and reasonable! Merging this one in! |
This PR addresses the concerns raised in issue #193 for the time frequency analysis submodule, as well as the semantic issue raised in issue #196. Specifically,
compute_wavelet_transform
now hasnorm
as a kwarg to specify what normalization to use. It defaults tosss
, or the L2 norm. Moreover, the output has been transposed so that frequency is plotted on the y-axis, and time on the x-axis. This matches convention for wavelet transforms in other packages.Update the test for
compute_wavelet_transform
to average across the appropriate axis.We now wrap the instantaneous phase vector using
np.unwrap
inside the call offreq_by_time
.Update the docstring for
robust_hilbert
to reflect that the return value is the analytic signal, not the hilbert transform.