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

MRG: Fix multitaper #2934

Merged
merged 3 commits into from Feb 23, 2016
Merged

MRG: Fix multitaper #2934

merged 3 commits into from Feb 23, 2016

Conversation

larsoner
Copy link
Member

Closes #2931.

Upstream bug nipy/nitime#140.

@larsoner larsoner changed the title FIX: Fix multitaper MRG: Fix multitaper Feb 23, 2016
@larsoner
Copy link
Member Author

Ready for review/merge from my end.

@larsoner larsoner added this to the 0.12 milestone Feb 23, 2016
@@ -33,6 +33,9 @@ def test_dpss_windows():
assert_array_almost_equal(dpss, dpss_ni)
assert_array_almost_equal(eigs, eigs_ni)

dpss, _ = dpss_windows(245411, 4, 8, False, 1000)
assert_equal(dpss.shape[-1], 245411)
Copy link
Member

Choose a reason for hiding this comment

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

is this long to compute?

Copy link
Member Author

Choose a reason for hiding this comment

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

~1 sec. But we could get rid of it if you want. I'm + 0.5 on removing it but I was afraid of no-test complainers :)

Copy link
Member

Choose a reason for hiding this comment

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

@larsoner
Copy link
Member Author

@agramfort done. Merge once CIs are happy?

@dengemann
Copy link
Member

@Eric89GXL what are the consequences of this bug? Is it in the category {hashtag}recompute_your_results?

@larsoner
Copy link
Member Author

The bug made it so that if you used really long signals you'd get a dimension error during multiplication. So if you haven't hit it, then it probably didn't affect you.

I also included another tweak that appeared upstream that determined when to flip the multitaper. I'm not 100% sure of the consequences there, but I'd expect them to be minor.

@larsoner
Copy link
Member Author

@dengemann okay to merge for you?

@dengemann
Copy link
Member

Yes!

@agramfort
Copy link
Member

+1 for MRG

agramfort added a commit that referenced this pull request Feb 23, 2016
@agramfort agramfort merged commit fee3155 into mne-tools:master Feb 23, 2016
@larsoner larsoner deleted the fix-filt branch February 23, 2016 16:08
@larsoner
Copy link
Member Author

Thanks for the quick reviews

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants