Skip to content

spike_amplitudes description#3925

Merged
alejoe91 merged 4 commits into
SpikeInterface:mainfrom
pas-calc:patch-1
May 19, 2025
Merged

spike_amplitudes description#3925
alejoe91 merged 4 commits into
SpikeInterface:mainfrom
pas-calc:patch-1

Conversation

@pas-calc
Copy link
Copy Markdown
Contributor

documentation
#3920

outputs : "numpy" | "by_unit", default: "numpy"
The output format, either concatenated as numpy array or separated on a per unit basis
peak_sign : "pos" or "neg"
to get the extremum_channels_indices
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Did you copy this from somewhere? :) We should rewrite that. The peak is whether you are looking for negative spikes (standard for extracellular potentials) or positive spikes (true for intracellular but a little contentious for extracellular experiments--I fall in that camp that things these are axonal).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In this case, I was looking for the class and params to see which variables are actually used.
(If I understood the construction with set_params and the parent class correctly, because the init doesn't have arguments. Which are the arguments of compute_spike_amplitudes)

Even though this extension is for individual spikes, I thought it would make sense to have a similar argument like for templates.

def get_template_amplitudes(

This allows for both a "peak_sign" and "mode".
Methods like "ptp" or energy(L2 norm) as used for waveform thresholding (sparsity) are/should not be applied on individual spikes' waveforms?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yeah it's just the _set_params.

I was saying you did it right, but we should rewrite the argument to explain it better :) The end-user might be confused how a peak_sign gets the extremum_channels_indices so instead we should explain what "peak" means. The internal details are less important vs the overall point of the argument.

Our documentation in general is written with the devs in mind, but we are slowly converting it to user facing docs as the project is now being used by a continuum of users from beginner to advanced. Does that make sense?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes I see, and I appreciate this to make the docs more understandable for a broad audience. The technical details however are also important. For example it was not clear for me whether spike amplitudes can be also computed by all extremum modes/peak_sign as it is the case for waveform/templates? That's why I looked up compute_spike_amplitudes in the docs.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can do both. We in general have two strategies, but you raise a good point that maybe we should be more consistent. If they can be explained in one sentence you could put them both on the argument/parameter line. If not then we support a Notes section at the bottom of the docstring (we follow numpydocs format) where you could talk about more technical details.

Copy link
Copy Markdown
Member

@zm711 zm711 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Just one tweak on my end.

Comment thread src/spikeinterface/postprocessing/spike_amplitudes.py Outdated
Comment thread src/spikeinterface/postprocessing/spike_amplitudes.py Outdated
@alejoe91 alejoe91 added the documentation Improvements or additions to documentation label May 19, 2025
@alejoe91 alejoe91 merged commit 03c818c into SpikeInterface:main May 19, 2025
14 of 15 checks passed
@pas-calc pas-calc deleted the patch-1 branch May 22, 2025 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants