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

[Bug] Incorrect Output in time_histogram for Single Unit Spike Train with output='rate' #648

Open
utwrd opened this issue Nov 13, 2024 · 2 comments · May be fixed by #650
Open

[Bug] Incorrect Output in time_histogram for Single Unit Spike Train with output='rate' #648

utwrd opened this issue Nov 13, 2024 · 2 comments · May be fixed by #650
Labels
bug Indicates an unexpected problem or unintended behavior enhancement Editing an existing module, improving something

Comments

@utwrd
Copy link

utwrd commented Nov 13, 2024

Description
When using time_histogram with output='rate', the result differs between using a single spike train (time_histogram(spiketrain, output='rate')) and wrapping it in a list (time_histogram([spiketrain], output='rate')). The correct result is obtained with the latter (time_histogram([spiketrain], output='rate')), but this discrepancy is confusing when we want to calculate the firing rate for each unit.

Expected Behavior
Both calls to time_histogram should yield the same result, regardless of whether a single spiketrain or a list of one spiketrain is provided.

Actual Behavior
time_histogram(spiketrain, output='rate') produces an incorrect result, whereas time_histogram([spiketrain], output='rate') provides the correct output. This discrepancy is due to how the function calculates the sample length based on the input type.

Cause
This issue arises from a condition in statistics.py, line 1191, where len(spiketrains) is used. When spiketrains is not provided as a list, len(spiketrains) mistakenly returns the length of the spike train sample rather than counting the number of spike trains, leading to an incorrect rate calculation.

Suggested Solution
To fix this issue, modify the code to ensure that spiketrains is always treated as a list, even if it contains only one unit. This can be done by wrapping spiketrain in a list if it is not already, ensuring consistent behavior for both single and multiple spike train inputs.

Steps to Reproduce

  1. Create a single spike train, spiketrain.
  2. Run time_histogram(spiketrain, output='rate') and observe the incorrect result.
  3. Wrap spiketrain in a list and run time_histogram([spiketrain], output='rate').
  4. Compare the results; the correct output is produced only when spiketrain is wrapped in a list.
@Moritz-Alexander-Kern
Copy link
Member

Moritz-Alexander-Kern commented Nov 14, 2024

Hey @utwrd ,
thanks for reporting this, I was able to reproduce this with the following minimal example:

import quantities as pq
from elephant.statistics import time_histogram
from neo.core import SpikeTrain

# Create a single spike train
spiketrain = 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 = time_histogram(spiketrain, output='rate', bin_size=0.5 * pq.s)
print("Histogram (direct):", histogram_direct)

# Wrap spiketrain in a list and run time_histogram
histogram_wrapped = time_histogram([spiketrain], output='rate', bin_size=0.5 * pq.s)
print("Histogram (wrapped):", histogram_wrapped)

Which yields:

Histogram (direct): [[0.4]
 [0.4]
 [0.4]
 [0.4]
 [0.4]
 [0. ]] 1/s
Histogram (wrapped): [[2.]
 [2.]
 [2.]
 [2.]
 [2.]
 [0.]] 1/s

Using elephant == 1.1.1.

This example confirms that the result for a single SpikeTrain is incorrect when using the time_histogram function.

The documentation specifies that only a list of spike trains is accepted as input, but this might be easily overlooked.
So indeed, this is an issue that should be addressed.

To resolve this, I see a couple of options:

  1. Raise a TypeError if something other than a list is passed to the function.
  2. Handle the case for a single spike train appropriately (as suggested by @utwrd )
  3. ?

Thanks you again for bringing this to our attention! 🎉

Documentation:

@Moritz-Alexander-Kern Moritz-Alexander-Kern added the bug Indicates an unexpected problem or unintended behavior label Nov 14, 2024
@Moritz-Alexander-Kern
Copy link
Member

  • decided on option 2
  • extend documentation accordingly

@Moritz-Alexander-Kern Moritz-Alexander-Kern added enhancement Editing an existing module, improving something bug Indicates an unexpected problem or unintended behavior and removed bug Indicates an unexpected problem or unintended behavior labels Nov 18, 2024
Moritz-Alexander-Kern added a commit to INM-6/elephant that referenced this issue Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior enhancement Editing an existing module, improving something
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants