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

SpikeMonitor.spike_trains returns spikes out of order #725

Closed
thesamovar opened this Issue Jun 30, 2016 · 6 comments

Comments

Projects
None yet
2 participants
@thesamovar
Member

thesamovar commented Jun 30, 2016

If you use t=SpikeMonitor.spike_trains(i) then t will not be in sorted order. It seems that we use some slightly strange code in EventMonitor.values with unique that isn't guaranteed to respect order.

@thesamovar thesamovar added the bug label Jun 30, 2016

@thesamovar thesamovar added this to the 2.0 milestone Jun 30, 2016

@mstimberg

This comment has been minimized.

Show comment
Hide comment
@mstimberg

mstimberg Jun 30, 2016

Member

Yikes, we should have a test for that! The recorded data should already be ordered by time (at least for each index) so possibly replacing:

sort_indices = np.argsort(indices)

in EventMonitor.values
by

sort_indices = np.argsort(indices, kind='mergesort')

is already enough (the default sort algorithm is not stable). Or maybe we should sort on index + time to be safe?

Member

mstimberg commented Jun 30, 2016

Yikes, we should have a test for that! The recorded data should already be ordered by time (at least for each index) so possibly replacing:

sort_indices = np.argsort(indices)

in EventMonitor.values
by

sort_indices = np.argsort(indices, kind='mergesort')

is already enough (the default sort algorithm is not stable). Or maybe we should sort on index + time to be safe?

@thesamovar

This comment has been minimized.

Show comment
Hide comment
@thesamovar

thesamovar Jun 30, 2016

Member

I think using a stable sort should be fine, and numpy does guarantee mergesort to be stable.

Member

thesamovar commented Jun 30, 2016

I think using a stable sort should be fine, and numpy does guarantee mergesort to be stable.

@mstimberg

This comment has been minimized.

Show comment
Hide comment
@mstimberg

mstimberg Jul 5, 2016

Member

I tried to write a test case that fails for current master, but I did not manage to generate out-of-order spike trains. Did you observe this problem under any particular circumstances (e.g. C++ standalone with OpenMP)?

Member

mstimberg commented Jul 5, 2016

I tried to write a test case that fails for current master, but I did not manage to generate out-of-order spike trains. Did you observe this problem under any particular circumstances (e.g. C++ standalone with OpenMP)?

@thesamovar

This comment has been minimized.

Show comment
Hide comment
@thesamovar

thesamovar Jul 6, 2016

Member

Versions were: python 3.5.1 Anaconda 4.0.0 numpy 1.10.4 brian 2.0rc3. Not sure which codegen target but definitely not standalone (so I don't think it's an OpenMP thing).

Member

thesamovar commented Jul 6, 2016

Versions were: python 3.5.1 Anaconda 4.0.0 numpy 1.10.4 brian 2.0rc3. Not sure which codegen target but definitely not standalone (so I don't think it's an OpenMP thing).

@thesamovar

This comment has been minimized.

Show comment
Hide comment
@thesamovar

thesamovar Jul 6, 2016

Member

Windows, probably 64 bit.

Member

thesamovar commented Jul 6, 2016

Windows, probably 64 bit.

mstimberg added a commit that referenced this issue Jul 6, 2016

Use a stable sort algorithm so that `Spikemonitor.spike_train` return…
…s sorted spike trains for each neuron

Fixes #725

@mstimberg mstimberg self-assigned this Jul 6, 2016

@mstimberg

This comment has been minimized.

Show comment
Hide comment
@mstimberg

mstimberg Jul 6, 2016

Member

Um, it turned out that I couldn't trigger the bug because my example list was too short -- numpy's quicksort implementation switches to insertion sort for partitions < 15 elements, so the sorting was necessarily stable. I searched slightly longer random sequences until I found one that leads to an unstable sort and as expected, recording such a sequence with SpikeMonitor led to the bug you observed. I pushed the fix and I'll open the PR in a sec.

Member

mstimberg commented Jul 6, 2016

Um, it turned out that I couldn't trigger the bug because my example list was too short -- numpy's quicksort implementation switches to insertion sort for partitions < 15 elements, so the sorting was necessarily stable. I searched slightly longer random sequences until I found one that leads to an unstable sort and as expected, recording such a sequence with SpikeMonitor led to the bug you observed. I pushed the fix and I'll open the PR in a sec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment