Removed decorators. Use rfft instead of fft for non-negative frequencies

Eden-Kramer-Lab / spectral_connectivity

Frequency domain estimation and functional and directed connectivity analysis tools for electrophysiological data

GNU General Public License v3.0

121 stars 43 forks source link

Removed decorators. Use rfft instead of fft for non-negative frequencies #10

Closed dizcza closed 4 years ago

dizcza commented 5 years ago

I'm thinking of using your package in https://github.com/NeuralEnsemble/elephant. To learn your package, I started with boiling down the complexity of decorators and properties. Here is a list of updates for you to consider to squash and merge.

fft -> rfft for non-negative frequencies
removed _protected duplicated fields in Multitaper
lazydecorator -> lru_cache in Connectivity
removed .keys() function that triggers error when you do dir(multitaper) or dir(connectivity)
updated examples accordingly (the only change was to rename .power() to .power)
use nice python 3.6+ string formatting

edeno commented 5 years ago

Hi @dizcza,

Thanks for the pull request! I will take a look at these as soon as I can.

coveralls commented 4 years ago

Coverage increased (+0.9%) to 75.047% when pulling 848b50fae0577c15276125441dd4cd87e842a70c on dizcza:master into a0d02a4af1cd711e510928b99f32f2e0256ca627 on Eden-Kramer-Lab:master.

edeno commented 4 years ago

A couple of quick comments:

There are a lot of different changes here. Some stylistic and some more functional. It would be helpful to maybe break this up into a couple of PRs with some justification for why you think they should be changes. For example, f-strings, while nice, are a stylistic change and also requiring a whole new minor version of python. I do like f-strings an I'm inclined to be okay with it, but it's sort of fundamentally different than the other stuff.
While I agree that computing the entire cross-spectral matrix with negative frequencies isn't ideal, one major thing is pairwise_spectral_granger_prediction, and possibly a couple others depends on a cross-spectral matrix with negative frequencies. So using the rfft everywhere will break these. It is probably a better move to compute the full cross-spectral matrix only when needed and otherwise compute only the one with non-negative frequencies.

dizcza commented 4 years ago

Right, I see the difference and, indeed, granger looks wrong with rfft and irfft only. Well, that means my PR does not make sense because I thought we always need real part only.

edeno commented 4 years ago

Hi @dizcza --

Could we cherry pick these commits you made:

lazydecorator -> lru_cache in Connectivity
removed .keys() function that triggers error when you do dir(multitaper) or dir(connectivity)

I could do them myself, but since they are your work, and I would like them in the library....

dizcza commented 4 years ago

Ok, I'll do it later

edeno commented 4 years ago

Thank you!