spike_amplitudes description#3925
Conversation
for more information, see https://pre-commit.ci
| 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 |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
zm711
left a comment
There was a problem hiding this comment.
Thanks for the PR. Just one tweak on my end.
documentation
#3920