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

TVB-2371 Implement signal energy computation #81

Closed
wants to merge 4 commits into from

Conversation

kimonoki
Copy link

@kimonoki kimonoki commented Aug 3, 2018

The energy is filtered and computed based on the time window size in the time series datatype.
The result is calculated for every time point in the data. For the time window length that will exceed the last time point, the energy is computed to the last time point.
For example for time data [1,2,3] and length 2 the last energy will only calculate at time 3.

@maedoc
Copy link
Member

maedoc commented Aug 3, 2018

The contribution is appreciated but implementation is not quite what I'd expect. It would be far more efficient to window the time series, apply a window function, FFT the windows, and sum over coefficients in the frequency band of interest. You might look at the coherence analyzer to get started.

Please avoid nested functions, and be sure to test your code, as I'm quite sure line 388 is wrong here

         "Length": self.sample_period * self.read_data_shaperead_data_shape('data')[0]}

Copy link
Member

@maedoc maedoc left a comment

Choose a reason for hiding this comment

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

Overall this is a nice addition. Please make the filter frequencies parameters exposed to the user, and don't hard code the filter coefficients.

@maedoc
Copy link
Member

maedoc commented Feb 6, 2020

This PR is getting stale without response to my review, and our development has moved to tvb-root repo. If you want to take this up again please send a fresh PR there.

@maedoc maedoc closed this Feb 6, 2020
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.

2 participants