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

Calculation of the spike-triggered phase and amplitude #121

Merged
merged 11 commits into from
Apr 4, 2018

Conversation

mdenker
Copy link
Member

@mdenker mdenker commented Oct 19, 2017

This is a first contribution towards tools to work with measures based on phase synchronization analysis. This function extracts the phase and amplitude of a signal at the times of spikes.

The function could use more unit tests, and there is certainly a lot of room for optimization, but before continuing I would like to know if this function would cover the required basic functionality?

@JuliaSprenger I think you may have similar code -- could you check in how far this is similar in functionality?

@mdenker mdenker added the enhancement Editing an existing module, improving something label Oct 19, 2017
@coveralls
Copy link
Collaborator

coveralls commented Oct 19, 2017

Coverage Status

Coverage decreased (-0.2%) to 86.808% when pulling b76a4ad on INM-6:feature/spiketriggeredphase into 1595c70 on NeuralEnsemble:master.

@JuliaSprenger
Copy link
Member

I this the code covers the basic requirements and the tests work on my side.

The code I used for calculating was partially overlapping with the current STA implementation at that time since collection the individual phases at the spike time is similar to collecting the LFP values at the spike times.

I suggest to adjust the behaviour of the spike_triggered_phase function to make it more similar to the sta in terms of reordering the arguments. Do you think it makes sense to include a 'window' option in the future? In that case the code would be again partially similar to the STA implementation (lines 150-165).

@mdenker mdenker added this to the Release 0.5 milestone Mar 27, 2018
@mdenker
Copy link
Member Author

mdenker commented Apr 3, 2018

I think adding a window option is something to discuss -- however, maybe this should be modularized, such that there is a common way in which all such functions handle the cutting. Raised an issue #151.

Copy link
Contributor

@alperyeg alperyeg left a comment

Choose a reason for hiding this comment

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

Small changes needed, otherwise ok.

"""

# Convert inputs to lists
if type(spiketrains) is not list:
Copy link
Contributor

Choose a reason for hiding this comment

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

For type comparision please use instanceof

>>> f_sampling = 1 * pq.ms
>>> tlen = 100 * pq.s
>>> time_axis = np.arange(
0, tlen.rescale(pq.s).magnitude,
Copy link
Contributor

Choose a reason for hiding this comment

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

In the example you do not need to resacle.

start = [elem.t_start for elem in hilbert_transform]
stop = [elem.t_stop for elem in hilbert_transform]

result_phases = [[] for _ in range(num_spiketrains)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to create empty lists? Can't you use later on extend or just numpy arrays while intializing?

for i, entry in enumerate(result_times):
result_times[i] = pq.Quantity(entry, units=entry[0].units).flatten()

return (result_phases, result_amps, result_times)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove redundant brackets.

@alperyeg alperyeg merged commit 1b45ecd into NeuralEnsemble:master Apr 4, 2018
@mdenker mdenker deleted the feature/spiketriggeredphase branch April 11, 2018 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Editing an existing module, improving something
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants