Fix postsynaptic trace edge case #1137

clinssen · 2019-02-27T20:45:13Z

This fixes #1034.

With props to my fellow sleuth @suku!

Background:

The synapse, when handling an incoming presynaptic spike at time t_pre_spike in send(), requests postsynaptic history within the interval t_prev_pre_spike - dendritic_delay < t ≤ t_pre_spike - dendritic_delay. Spikes in this interval will have their access counters incremented. The postsynaptic neuron, in Archiving_Node::set_spiketime(), will purge all spikes from the buffer which have been accessed by all connecting synapses.

A minimal reproducing example of the bug has presynaptic spike times at [2, 3, 10] and postsynaptic at [1, 2, 3] (all times in ms). All delays (axonal, dendritic, synaptic) are equal to 1. The failure occurs as follows: The first postsynaptic spike at t = 2 is added to the postsynaptic buffer. The first presynaptic spike at t = 3 causes the synapse to access the postsynaptic buffer. This increments the access counter of the spike at t = 2, marking it for deletion. An extra postsynaptic spike at t = 3 is necessary because the history always needs to have at least two entries if a spike is to be deleted. Next, the postsynaptic neuron spikes at t = 4, which causes the spike to be deleted from the buffer. When another presynaptic spike then arrives at t = 4, the get_K_value() function is called on the postsynaptic partner to obtain the trace at t = 3, but the necessary spike at time 2 has now been deleted. Consequently, zero is erroneously returned for the trace value.

How this PR fixes things:

Recall that spike buffer pruning occurs upon firing of the neuron at time t_sp in Archiving_Node::set_spiketime(). A new condition for pruning is added: only remove a spike if there is another, later spike, that is strictly more than delay away from t_sp (https://github.com/clinssen/nest-simulator/blob/76a71602edc929116a464d4e8ab34c8cc00b5242/nestkernel/archiving_node.cpp#L210). This ensures that when get_K_value() is called to obtain the trace at time t - delay, a spike strictly earlier than this is still in the buffer.

To obtain the maximum delay value across all synapses, a new private member variable max_delay_ is added in Archiving_Node, which is updated whenever Archiving_Node::register_stdp_connection() is called.

Notes for reviewers:

The trace value as calculated by get_K_value() is cached in the Archiving_Node under the new private member variable trace_. This is mostly for debugging purposes. If this affects performance, I suggest tying it to the "with-debug" NEST build flag.
I am not happy about the large tolerance that seems to be necessary when comparing the NEST trace values with the reference values obtained through Python. See: https://github.com/clinssen/nest-simulator/blob/76a71602edc929116a464d4e8ab34c8cc00b5242/pynest/nest/tests/test_post_trace.py#L35

…member variable

…estep

…thin one (dmin) timestep

… buffer

hakonsbm

Thank you for finding and fixing this bug! I tried benchmarking with the brunel_ps.sli example, and it shows no significant performance loss from caching in trace_. In the C++ part there is some formatting that needs fixing, see below.

The added testfile, test_post_trace.py, is quite messy. Some suggestions:

The plotting isn't required for running the unittest, so I suggest to have it separate from the test. perhaps commented out below together with something like a "make_plot" function. It also needs some refactoring, but as it's only for debugging that's not a priority.
The helper functions run_post_trace_test_nest_, run_post_trace_test_python_reference_, and nest_trace_matches_ref_trace can be moved out of the PostTraceTestCase class. One possibility is to create a separate class containing them as methods, which can reduce the number of arguments.

hakonsbm · 2019-03-13T13:35:17Z

nestkernel/archiving_node.cpp

  , last_spike_( -1.0 )
  , Ca_t_( 0.0 )
  , Ca_minus_( 0.0 )
  , tau_Ca_( 10000.0 )
  , beta_Ca_( 0.001 )
+  , trace_(0.)


Variables should be initialized in the same order as they are declared in the class. So trace_ should be initialized directly after max_delay_.

hakonsbm · 2019-03-13T13:36:02Z

nestkernel/archiving_node.cpp

  , last_spike_( n.last_spike_ )
  , Ca_t_( n.Ca_t_ )
  , Ca_minus_( n.Ca_minus_ )
  , tau_Ca_( n.tau_Ca_ )
  , beta_Ca_( n.beta_Ca_ )
+  , trace_(n.trace_)


Same as above, move initialization of trace_ to directly after max_delay_.

hakonsbm · 2019-03-13T13:43:11Z

nestkernel/archiving_node.cpp

  }
+
+  // search for the latest post spike in the history buffer that came strictly before `t`


Line is too long (>80).

hakonsbm · 2019-03-13T13:43:17Z

nestkernel/archiving_node.cpp

  }
-  return 0;
+
+  // this case occurs when the trace was requested at a time precisely at or before the first spike in the history


Line is too long (>80).

hakonsbm · 2019-03-13T13:44:48Z

nestkernel/archiving_node.cpp

-    // except the penultimate one. we might still need it.
+    // only remove a spike if:
+    // - its access counter indicates it has been read out by all connected STDP synapses, and
+    // - there is another, later spike, that is strictly more than (max_delay_ + eps) away from the new spike (at t_sp_ms)


Keep line lengths under 80 characters.

hakonsbm · 2019-03-13T14:25:26Z

pynest/nest/tests/test_post_trace.py

+#
+# NEST is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See thed


"thed" -> "the"

hakonsbm · 2019-03-13T14:27:27Z

pynest/nest/tests/test_post_trace.py

+import scipy as sp
+import scipy.stats
+import matplotlib.pyplot as plt
+import matplotlib.ticker as plticker


math is unused, and sp.stats is used throughout, so no need to import scipy.stats.

hakonsbm · 2019-03-13T14:29:29Z

pynest/nest/tests/test_post_trace.py

+
+        nest.set_verbosity("M_WARNING")
+
+        post_weights = {'parrot': [], 'parrot_ps': []}


post_weights isn't used anywhere.

hakonsbm · 2019-03-13T14:31:09Z

pynest/nest/tests/test_post_trace.py

+                      'delay': delay})
+
+        # get STDP synapse
+        syn_ps = nest.GetConnections(source=pre_parrot_ps,


syn_ps isn't used after assigning it.

hakonsbm · 2019-03-13T14:34:54Z

pynest/nest/tests/test_post_trace.py

+            t = trace_nest_t[i]
+            print("* Finding ref for NEST timepoint t = "
+                  + str(t) + ", trace = " + str(trace_nest[i]))
+            for i_search, t_search in enumerate(


Doesn't look like i_search is used anywhere, so no need to enumerate.

heplesser

@clinssen Please minimize the test to a pure test script with no plotting. The test should not contain anything that is not required for testing. Also, move it to the regressiontest directory and rename it issue-1034. Our test system handles python scripts in all test directories. If you want to keep the plotting for documentation purposes, a notebook under doc/model_details would be a good place.

…nd Jupyter notebook

clinssen · 2019-03-26T11:02:59Z

Gentlemen, many thanks for your excellent suggestions. The new series of commits should address all of the points you raised. Additionally, I've been able to tighten the (absolute and relative) tolerances from 1E-2 to 1E-3 by making some modifications in how the reference timeseries is generated.

hakonsbm

Thanks for the changes, looks a lot better now! I have a couple remarks:

I'm not sure if we need a copyright notice in notebooks (there aren't any in the others)
It doesn't look like the added NEST name pre_trace is used for anything. It can probably be removed.

I also have some suggestions about the python test, see below.

hakonsbm · 2019-03-26T12:34:07Z

nestkernel/archiving_node.cpp

-      if ( history_.front().access_counter_ >= n_incoming_ )
+      const double next_t_sp = history_[ 1 ].t_;
+      if ( history_.front().access_counter_ >= n_incoming_
+        && t_sp_ms - next_t_sp > max_delay_


Use and instead of &&.

hakonsbm · 2019-03-26T13:26:52Z

testsuite/regressiontests/issue-1034.py

+        nest.Connect(
+            pre_parrot_ps + post_parrot_ps,
+            spikes
+        )


Why not just nest.Connect(pre_parrot_ps + post_parrot_ps, spikes) in one line?

hakonsbm · 2019-03-26T13:27:57Z

testsuite/regressiontests/issue-1034.py

+        for sp_idx in range(n_spikes):
+            t_sp = self.post_spike_times_[sp_idx] \
+                   + self.delay_ \
+                   + self.dendritic_delay_


It is recommended to use parentheses to break lines instead of backslash.

hakonsbm · 2019-03-26T13:31:30Z

testsuite/regressiontests/issue-1034.py

+                                                  / self.tau_minus_)
+
+        n_spikes = len(self.pre_spike_times_)
+        for sp_idx in range(n_spikes):


Loop over spike times, not indices.

hakonsbm · 2019-03-26T13:32:36Z

testsuite/regressiontests/issue-1034.py

+        """
+
+        n_timepoints = len(trace_nest_t)
+        for i in range(n_timepoints)[1:]:


Loop over trace_nest_t, not indices.

hakonsbm · 2019-03-26T13:41:31Z

testsuite/regressiontests/issue-1034.py

+                    break
+
+            if (not traces_match) \
+               and i_search == len(self.pre_spike_times_) - 1:


Use parentheses to break lines.

hakonsbm · 2019-03-26T13:43:53Z

testsuite/regressiontests/issue-1034.py

+                            post_spike_times2,
+                            pre_spike_times2]
+
+        for spike_times_idx in range(len(pre_spike_times)):


Instead of iterating the index, iterate the spike times with zip. So it would become something like

for pre_spike_time, post_spike_time in zip(pre_spike_times, post_spike_times):

hakonsbm · 2019-03-26T13:44:27Z

testsuite/regressiontests/issue-1034.py

+                  + ", ".join([str(t) for t in post_spike_times]) + "]")
+
+            for delay in delays:
+                dendritic_delay = delay


dendritic_delay is unused.

hakonsbm · 2019-03-26T13:44:50Z

testsuite/regressiontests/issue-1034.py

+                    tau_minus=tau_minus,
+                    trace_match_atol=1E-3,
+                    trace_match_rtol=1E-3)
+                assert test.nest_trace_matches_python_trace()


Use self.assertTrue().

hakonsbm · 2019-03-26T13:46:59Z

testsuite/regressiontests/issue-1034.py

+
+
+if __name__ == "__main__":
+    runner = unittest.TextTestRunner(verbosity=99)


Verbosity only goes up to 2.

heplesser · 2019-03-26T20:00:10Z

@clinssen

I'm not sure if we need a copyright notice in notebooks (there aren't any in the others)

I would leave the copyright notice out of the notebook for now, in accordance with the existing notebooks.

testsuite/regressiontests/issue-1034.py

…inal

hakonsbm · 2019-04-05T13:30:48Z

@clinssen Thanks for the changes! There are a couple comments in the python test you may have overlooked:

L131, use parentheses to break lines here
Can the if-tests on lines L192 and L207 be merged together?

I'll give the thumbs up when these comments are resolved.

…inal

heplesser · 2019-04-08T11:36:09Z

@clinssen Thank you for the notebook. Could you add some more text in the final part of the notebook explaining the figures, especially the synaptic trace from NEST? Is the behaviour shown in the graphs the behaviour before or after this fix has been applied? If it is the behaviour after the fix, then why do we still see this difference?

clinssen · 2019-04-08T11:38:24Z

The latest commits should address all of your comments. @hakonsbm: the if-tests cannot be merged, because I'm using the pattern:

valid = test_if_valid1()
if not valid:
    valid = test_if_valid2()
if not valid:
    ...

clinssen · 2019-04-08T11:41:03Z

@heplesser: I posted my previous comment on a stale version of this page; please hang tight!

hakonsbm

@clinssen Fair enough, I won't nitpick any more. 👍

C.A.P. Linssen added 20 commits November 28, 2018 20:27

fixed bug and added testbench for postsynaptic trace function

febaad5

added assertion failure upon fall-through case

d089391

updated the example

a725a1b

refactor postsynaptic trace demo script to unit test

85fa1eb

replace history-based calculation of postsynaptic trace with private …

2ca0a85

…member variable

update unit test to account for multiple spikes within one (dmin) tim…

fc879a2

…estep

update postsynaptic trace unit test to account for multiple spikes wi…

15825ec

…thin one (dmin) timestep

added debugging code to archiving node

38010cb

update postsynaptic trace unit test to account for multiple spikes wi…

abea27a

…thin one (dmin) timestep

add new, stricter condition for removing spikes from the postsynaptic…

aea2949

… buffer

added new parameter to register_stdp_connection() to pass synaptic delay

cf2c991

refactor testbench

ee90db9

postsynaptic trace fixes for nest#1034; updated testbench

e7e55ac

postsynaptic trace fixes for nest#1034: cleanup

41060e3

postsynaptic trace fixes for nest#1034: cleanup

fd2a9d7

postsynaptic trace fixes for nest#1034: cleanup

7d4fb0e

postsynaptic trace fixes for nest#1034: cleanup

ba09243

postsynaptic trace fixes for nest#1034: cleanup

76a7160

pep8

73fc9c3

clang-format

5a91180

heplesser requested review from jakobj and hakonsbm March 13, 2019 07:33

hakonsbm suggested changes Mar 13, 2019

View reviewed changes

C.A.P. Linssen added 2 commits March 13, 2019 18:02

homogenise comments/formatting between get_K_value() and get_K_values()

4a11b18

fix member variable initialisation order

a868258

heplesser requested changes Mar 25, 2019

View reviewed changes

heplesser removed the request for review from jarsi March 25, 2019 12:35

C.A.P. Linssen added 6 commits March 25, 2019 17:15

split postsynaptic trace testbench into pure (regression) test part a…

ec11b5f

…nd Jupyter notebook

clean up postsynaptic trace regression test

ee8cd49

clean up (PEP8) postsynaptic trace regression test

c662f2e

removed check_stack from postsynaptic trace regression test

40a1a28

minor cleanup of postsynaptic trace regression test and Jupyter notebook

d1c8543

add reference to Jupyter notebook in regression test

3f87224

hakonsbm suggested changes Mar 26, 2019

View reviewed changes

heplesser reviewed Mar 26, 2019

View reviewed changes

testsuite/regressiontests/issue-1034.py Show resolved Hide resolved

C.A.P. Linssen added 2 commits April 1, 2019 11:52

change verbosity level to the maximum defined

90b5c28

Merge remote-tracking branch 'upstream/master' into traces-refactor_f…

39ea006

…inal

heplesser added this to the NEST-2.18 milestone Apr 1, 2019

C.A.P. Linssen added 2 commits April 1, 2019 12:23

remove unused NEST name 'pre_trace'

d7ed53b

minor refactoring based on PR comments

eefa1aa

C.A.P. Linssen added 4 commits April 8, 2019 10:45

Merge remote-tracking branch 'upstream/master' into traces-refactor_f…

389e715

…inal

add documentation header; minor refactoring based on PR comments

320c659

remove stray line left over from refactoring

acd122f

remove copyright header from Jupyter notebook

bb5b275

hakonsbm approved these changes Apr 8, 2019

View reviewed changes

add figure legend

0eead87

heplesser approved these changes Apr 8, 2019

View reviewed changes

heplesser merged commit 58b53ba into nest:master Apr 8, 2019

JanVogelsang mentioned this pull request Jul 26, 2022

get_K_value() can still return 0 erroneously in edge case #2437

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix postsynaptic trace edge case #1137

Fix postsynaptic trace edge case #1137

clinssen commented Feb 27, 2019

hakonsbm left a comment

hakonsbm Mar 13, 2019

hakonsbm Mar 13, 2019

hakonsbm Mar 13, 2019

hakonsbm Mar 13, 2019

hakonsbm Mar 13, 2019

hakonsbm Mar 13, 2019

hakonsbm Mar 13, 2019

hakonsbm Mar 13, 2019

hakonsbm Mar 13, 2019

hakonsbm Mar 13, 2019

heplesser left a comment

clinssen commented Mar 26, 2019

hakonsbm left a comment

hakonsbm Mar 26, 2019

hakonsbm Mar 26, 2019

hakonsbm Mar 26, 2019

hakonsbm Mar 26, 2019

hakonsbm Mar 26, 2019

hakonsbm Mar 26, 2019

hakonsbm Mar 26, 2019

hakonsbm Mar 26, 2019

hakonsbm Mar 26, 2019

hakonsbm Mar 26, 2019

heplesser commented Mar 26, 2019

hakonsbm commented Apr 5, 2019

heplesser commented Apr 8, 2019

clinssen commented Apr 8, 2019

clinssen commented Apr 8, 2019

hakonsbm left a comment

		}

		// search for the latest post spike in the history buffer that came strictly before `t`


		nest.set_verbosity("M_WARNING")

		post_weights = {'parrot': [], 'parrot_ps': []}



		if __name__ == "__main__":
		runner = unittest.TextTestRunner(verbosity=99)

Fix postsynaptic trace edge case #1137

Fix postsynaptic trace edge case #1137

Conversation

clinssen commented Feb 27, 2019

Background:

How this PR fixes things:

Notes for reviewers:

hakonsbm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heplesser left a comment

Choose a reason for hiding this comment

clinssen commented Mar 26, 2019

hakonsbm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heplesser commented Mar 26, 2019

hakonsbm commented Apr 5, 2019

heplesser commented Apr 8, 2019

clinssen commented Apr 8, 2019

clinssen commented Apr 8, 2019

hakonsbm left a comment

Choose a reason for hiding this comment