genn-team / pynn_genn

PyNN interface to GeNN
GNU General Public License v2.0
9 stars 3 forks source link

Recording spikes; ids seem to need sorting #45

Open chanokin opened 4 years ago

chanokin commented 4 years ago

I was getting weird spikes out of simulation, they appeared to be shifted by neuron id. After adding a sort to the new_ids set it looks like this is corrected. https://github.com/genn-team/pynn_genn/blob/0774e22ef99c11325befe05f2dd7072b30e60693/pynn_genn/recording.py#L40-L41 Should become

self.id_data_idx_map.update({idd - self.start_id: i + iimap_len
                                     for i, idd in enumerate(sorted(new_ids))})

I have been unable to generate a small test case, though I've observed this in my big simulations repeatedly. Also, I can only verify this with SpikeSourceArrays as I can compare the input to the output.

neworderofjamie commented 4 years ago

That is interesting! can you try and figure out why sorting might be required? dictionaries in Python are unordered so I'd be a little concerned that this just works around a more fundamental problem with this module...However, I just had a look at the code and I honestly can no longer remember how it worked 😟

jamesturner246 commented 4 years ago

Just a side note. Ducts in python37 are now ordered.

On Wed, 12 Feb 2020, 13:30 neworderofjamie, notifications@github.com wrote:

That is interesting! can you try and figure out why sorting might be required? dictionaries in Python are unordered so I'd be a little concerned that this just works around a more fundamental problem with this module...However, I just had a look at the code and I honestly can no longer remember how it worked 😟

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/genn-team/pynn_genn/issues/45?email_source=notifications&email_token=ABUJDNSNNYYBFT5TBIYSFA3RCP2XJA5CNFSM4KTYQIAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELQYOYA#issuecomment-585205600, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABUJDNU5MQG56GWA4UPEIDDRCP2XJANCNFSM4KTYQIAA .

chanokin commented 4 years ago

From what I can understand, new_ids is a set constructed in PyNN from a Numpy array comming from pynn_genn (which is already sorted! :angry: ). Unfortunately, after the set is created, the ids are scrambled (shifted). The scrambled ids are then used to create a dictionary (id_data_idx_map) which relies on having the correct order (i.e. using enumerate)

It makes sense that, for a set, order does not matter but when mapping neuron-to-data ids, if the neuron ids are not properly sorted the neuron mapping points to the wrong data row. So, it looks like the problem comes from the using set to construct the dictionary, not de dictionary itself.

Annoyingly, I've only seen this in one of the populations in my experiments and, fortunately, it seems to only affect the recording side of things.