-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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: Fix bug with minimum phase filters #12507
Conversation
doc/changes/devel/12507.bugfix.rst
Outdated
compromised attenuation. Now ``phase="minimum"`` has the same length and comparable | ||
suppression as ``phase="minimum"``, and the old (incorrect) behavior can be achieved |
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.
compromised attenuation. Now ``phase="minimum"`` has the same length and comparable | |
suppression as ``phase="minimum"``, and the old (incorrect) behavior can be achieved | |
compromised attenuation. Now ``phase="minimum"`` has the same length and comparable | |
suppression as ``phase="zero"``, and the old (incorrect) behavior can be achieved |
tautology; I think this is the right repair but @larsoner please confirm
# that the falloff is not as steep (though here we use a temporary workaround | ||
# version vendored by MNE until https://github.com/scipy/scipy/pull/19706 is | ||
# merged and released): | ||
|
||
h_min = signal.minimum_phase(h) | ||
h_min = minimum_phase(h, half=False) |
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.
not sure we want to talk about "vendoring" in our tutorials. Seems like a detail users shouldn't need to know about. Maybe instead just mention which SciPy / MNE versions & params are necessary to get the correct behavior?
Fixed, can you see if the wording is good enough now @drammock ? |
Co-authored-by: Daniel McCloy <dan@mccloy.info>
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Daniel McCloy <dan@mccloy.info>
Closes #12267
No need to wait for the SciPy PR I think since the tests there (and here, including the updated tutorial) show this works correctly.