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

FIX: Kmax wrong, BW = bandwidth #105

Closed
wants to merge 1 commit into from
Closed

Conversation

mluessi
Copy link

@mluessi mluessi commented Sep 7, 2012

While integrating the DPSS window functions into mne-python, I found it confusing that NW is the "half normalized bandwidth" instead of just the normalized bandwidth. This also caused Kmax to be wrong (since "int(2 * NW)" assumes that NW is the normalized bandwidth, not half-bandwidth).

@arokem
Copy link
Member

arokem commented Sep 7, 2012

Hi Martin,

First of all - thanks a lot for taking a look at this!

I think that the original intent was to be as similar as possible to the
equivalent matlab function (dpss), such that a call of:

nitime.algorithms.dpss_windows(100,2,4)

would give a result almost equal to the call:

dpss(100,2)

Indeed, with your change to the code, test_dpss_matlab is now failing (and,
in my hands, a couple of other related tests as well...). Those would
probably need to change to accomodate this kind of change, but I am
reluctant to diverge from the matlab-equivalent call. @mtrumpis might also
be able to say a bit more on why people use half-bandwidth rather than
bandwidth (I agree that it can be confusing).

Also - I am not sure that I understand the comment about Kmax being wrong.
Could you please say a bit more about that?

Thanks again!

On Fri, Sep 7, 2012 at 7:13 AM, Martin Luessi notifications@github.comwrote:

While integrating the DPSS window functions into mne-python, I found it
confusing that NW is the "half normalized bandwidth" instead of just the
normalized bandwidth. This also caused Kmax to be wrong (since "int(2 *

NW)" assumes that NW is the normalized bandwidth, not half-bandwidth).

You can merge this Pull Request by running:

git pull https://github.com/mluessi/nitime fix_bw_norm

Or view, comment on, or merge it at:

#105
Commit Summary

  • FIX: Kmax wrong, BW = bandwidth

File Changes

  • M nitime/algorithms/spectral.py (8)

Patch Links

@mluessi
Copy link
Author

mluessi commented Sep 7, 2012

I wasn't aware that the matlab dpss function requires the half-bandwidth. I does make sense to try to make the functions equivalent.

Regarding Kmax: In the Thomson "Jacknifing Multitaper Spectrum Estimates" article, NW is defined as "NW=B * T", where B is the bandwidth in Hz and T the sample duration in s. For this NW there are "K = floor( 2 * NW )" tapers. In nitime "NW_ni = NW / 2", so there should be "K = floor( 4 * NW_ni )" tapers.

It could be that the Thomson article is unclear and "B" is the half-bandwidth, while in nitime "BW" is the full bandwidth of the taper. In fact, in his 1982 paper, Thomson uses " 2 * N * W" tapers where W is the half-bandwidth, so nitime would be correct.

I'm not an an expert on multi-taper methods, please chime in if you know what is correct.

@mluessi
Copy link
Author

mluessi commented Sep 7, 2012

Ok, according to the matlab dpss documentation, what nitime is doing is indeed correct. Please disregard this PR.

@miketrumpis
Copy link
Contributor

Hi all, I was beginning to look at this when I just noticed all the back and forth. I suspected it was a matter of terminology--I like to use "half bandwidth" where some others like to say bandwidth.

Glad that's resolved.

Martin, let me direct your attention to an in-progress refactoring of that very code:

https://github.com/miketrumpis/nitime/blob/mt_fix_ups/nitime/algorithms/spectral.py

Aside from structural changes, there are some feature enhancements long overdue (NFFT never worked? what?). You can also now specify taper resolution in a mixed way, ie using either Hz or the unit-free NW term. Because I prefer NW

Just pushed a lingering commit, so the MTM part of the changes should be good to go. The remaining project is to implement the harmonic line detection F-test.

So I'll go ahead and close this up.

cheers
Mike

@miketrumpis miketrumpis closed this Sep 7, 2012
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