[ENH] time_histogram handles single neo.SpikeTrain
#650
Conversation
Moritz-Alexander-Kern
added
the
enhancement
Moritz-Alexander-Kern
left a comment
Moritz-Alexander-Kern
left a comment
There was a problem hiding this comment.
build wheels
kohlerca
left a comment
kohlerca
left a comment
There was a problem hiding this comment.
Overall, the proposed changes look good, and the code works as expected. I made small suggestions for improvement and to fix errors in the docstring.
| spiketrain = neo.SpikeTrain([0.1, 0.5, 1.0, 1.5, 2.0] * pq.s, t_stop=3.0 * pq.s) | ||
|
|
||
| # Run time_histogram with spiketrain directly and observe the incorrect result | ||
| histogram_direct = statistics.time_histogram(spiketrain, output='rate', bin_size=0.5 * pq.s) |
There was a problem hiding this comment.
It would be more complete to include the three possible output formats, not only 'rate,' to fully cover the changes and ensure correctness.
| neo.AnalogSignal | ||
| A `neo.AnalogSignal` object containing the histogram values. | ||
| `neo.AnalogSignal[j]` is the histogram computed between | ||
| A :class:`neo.core.SpikeTrain` object containing the histogram values. |
There was a problem hiding this comment.
The return is a Neo AnalogSignal object (line 1205)
| # 'mean': mean spike counts per spike train. | ||
| return pq.Quantity(bin_hist / len(spiketrains), | ||
| units=pq.dimensionless, copy=False) | ||
| return pq.Quantity(bin_hist / binned_spiketrain.shape[0], |
There was a problem hiding this comment.
Not covered in the additional unit test.
| between `t_start` and `t_stop` (both included) are considered in the | ||
| histogram. | ||
| If None, the maximum `t_start` of all `neo.SpikeTrain`s is used as | ||
| If None, the maximum `t_start` of all :class:`neo.core.SpikeTrain`s is used as |
There was a problem hiding this comment.
This is not formatted correctly in ReadTheDocs. A solution is to use
:class:`neo.core.SpikeTrain` objects
as the plural s seems to break the formatting.
| between `t_start` and `t_stop` (both included) are considered in the | ||
| histogram. | ||
| If None, the minimum `t_stop` of all `neo.SpikeTrain`s is used as | ||
| If None, the minimum `t_stop` of all :class:`neo.core.SpikeTrain` s is used as |
There was a problem hiding this comment.
Same suggestion for formatting as above in t_start. This is formatted as expected, but with the link to the class documentation, it is unreadable.
| If True, indicates whether all :class:`neo.core.SpikeTrain` objects should first | ||
| be binned to a binary representation (using the | ||
| `conversion.BinnedSpikeTrain` class) and the calculation of the | ||
| [:class:`elephant.conversion.BinnedSpikeTrain` class] and the calculation of the |
There was a problem hiding this comment.
With the enhancement provided by the class link, the brackets [ ... ] are not needed, and there is the missing closing parenthesis ).
3 participants
This pull request addresses issue #648 by updating the
statistics.time_histogramfunction to accept a singleneo.SpikeTrainobject as input.Previously, the function was limited to handling lists of spike trains, which led to incorrect results when a single spike train was provided. This enhancement ensures that the function now correctly computes the time histogram for both single and multiple spike trains.
Changes
neo.SpikeTraininput.time_histogramcopyparameter when creatingpq.Quantityobject