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

Remove deprecated models iaf_psc_alpha_canon and pp_pop_psc_delta #2294

Merged
merged 17 commits into from
Nov 30, 2022

Conversation

jougs
Copy link
Contributor

@jougs jougs commented Feb 15, 2022

Here's a small and hopefully non-controversial one removing deprecated models. I also cleaned up the handling of connection model flags a bit to get shorter and more readable lines.

@jougs jougs added S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. I: Behavior changes Introduces changes that produce different results for some users labels Feb 15, 2022
@jougs jougs added this to the NEST 3.3 milestone Feb 15, 2022
@jougs jougs added this to To do in Models via automation Feb 15, 2022
@jougs jougs self-assigned this Feb 15, 2022
@stinebuu stinebuu requested review from stinebuu and removed request for heplesser February 16, 2022 08:24
Copy link
Contributor

@stinebuu stinebuu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but I tested the two examples and they do not run for me. I think you need to change tau_syn to tau_syn_{ex, in} somewhere.

examples/nest/brunel_ps.sli Show resolved Hide resolved
examples/nest/ArtificialSynchrony.sli Show resolved Hide resolved
@jougs jougs requested a review from stinebuu February 18, 2022 09:04
Copy link
Contributor

@stinebuu stinebuu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! The examples take a bit more time to run (brunel_ps have a simulation time of 267.26 s with iaf_psc_alpha_ps and 94.99 s with the master version, likewise ArtificialSyncrony is also slower with the precise model), but I guess this is to be expected with a precise model? See also issue #2017 (the times there are longer, if I remember correctly I ran the examples during a hackathon, and had video running simultaneously, slowing everything down).

Copy link
Contributor

@clinssen clinssen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving on the condition that we are OK with the runtime performance regression. I wasn't privy to the discussion about why these models were marked as deprecated in the first place.

models/modelsmodule.cpp Outdated Show resolved Hide resolved
@jougs
Copy link
Contributor Author

jougs commented Feb 22, 2022

@stinebuu: many thanks for measuring execution times. I'm actually not sure if this slow-down is to be expected, as the deprecated (and now removed) model iaf_psc_alpha_canon was also a precise variant.

When replacing iaf_psc_alpha_canon with iaf_psc_alpha_ps I just followed the documentation of the former.

@suku248 can you please quickly comment on this?

@suku248
Copy link
Contributor

suku248 commented Mar 1, 2022

Regarding @stinebuu 's observation of slower runtimes when using iaf_psc_alpha_ps instead of the deprecated iaf_psc_alpha_canon: The implementations differ in the way they determine the precise location of a threshold crossing. While iaf_psc_alpha_canon used an interpolation method (precision depended on simulation resolution), iaf_psc_alpha_ps uses an iterative method (no dependence on sim res). Hanuschkin et al. (2010) did not observe any significant differences in runtime when comparing the two implementations for the IAF neuron model with exponential PSCs (see Fig. 5). Therefore, I would not have expected to see a substantial slowdown here. Maybe the iterative method doesn't converge as fast as it should be?

@jougs
Copy link
Contributor Author

jougs commented Mar 1, 2022

@suku248, many thanks for these insights.

I'm really not sure how to proceed here. @heplesser, @abigailm, @diesmann: any thoughts on this? Is there anyone currently working on the precise models who could investigate further?

@heplesser
Copy link
Contributor

I will have another look at the performance problems.

@diesmann
Copy link

diesmann commented Mar 2, 2022

Thanks HEP. Whether _ps or _canon are faster depends on the computation time step and the requested accuracy. The general observation is that for a given accuracy _ps is faster than _canon because _ps can reach this accuracy already at larger computation time steps. With a fixed computation time step and a low required accuracy _canon might be faster because there are fewer operations to be carried out. The Hanuschkin paper has the details on this relationship.

@stinebuu
Copy link
Contributor

I have been doing some digging. It seams like the problem is the use of propagator_31 and propagator_32 in iaf_psc_alpha_ps' propogate_ function. In canon, the propogate variables are calculated directly, while ps use propagator_31 and propagator_32.

When I use canon's way of doing things in the ps model (see the change here) I get the following times:

type time
brunel_ps.sli with ps model 101.99 sec
`brunel_ps.sli with canon 93.96 sec
ArtificialSyncrony with ps 5 min 59 sec
ArtificialSyncrony with canon 6 min 39 sec

(brunel_ps with original ps model takes 227.94 sec).

I will try updating propagator_31 and propagator_32 now so they are part of a class, where constants are pre-calculated, similar to what is done with canon.

@jougs
Copy link
Contributor Author

jougs commented Mar 18, 2022

Merging this still depends on more discussion about the performance implications. Therefore removing the milestone.

@jougs jougs removed this from the NEST 3.3 milestone Mar 18, 2022
@github-actions
Copy link

Pull request automatically marked stale!

@github-actions github-actions bot added the stale Automatic marker for inactivity, please have another look here label May 18, 2022
@terhorstd
Copy link
Contributor

Hi everyone, has more discussion lead to new insights? – Just a friendly ping.

@heplesser
Copy link
Contributor

I have now re-run benchmarks for the brunel_ps.sli model using the propagator implementation provided in #2385 using iaf_psc_alpha_canon (deprecated, to be removed) and iaf_psc_alpha_ps. Experiments were done on a MacBook Pro with 2 GHz Quad Core Intel Core i5 with Clang 14. Code is from heplesser@10f6434, which provides a new IAFPropagator class, except for the ..._alpha_ps version of brunel_ps.sli, which is taken from this PR. Simulation times are minima of three runs. For comparison, also results for the current master e49be2a with the old propagator implementation are given. The alpha_canon model uses linear interpolation to find the precise spike time.

Code Neuron model Simulation time
heplesser/nest-simulator@10f6434 iaf_psc_alpha_canon 41.4 s
heplesser/nest-simulator@10f6434 iaf_psc_alpha_ps 55.3 s
master@e49be2a iaf_psc_alpha_ps 78.9 s

The ps variant is thus about 25% slower than the canon variant, but for two good reasons:

  1. ps uses an iterative method to find the spike time, while canon uses linear interpolation
  2. ps supports different excitatory and inhibitory synaptic time constants

Notice also that the new IAFPropagator class provided by #2385 significantly improves performance for the ps model compared to current master.

I therefore believe that the qualities of the ps model justify removal of canon once #2385 is merged.

@diesmann
Copy link

In

Morrison A., Straube S., Plesser HE., Diesmann M. (2007) Exact Subthreshold Integration with Continuous Spike Times in Discrete-Time Neural Network Simulations. Neural Computation 19:47–79.
DOI: 10.1162/neco.2007.19.1.47.

We argue that stating the time required to propagate the implementation of a particular neuron model has no meaning. If the integration error is not specified an implementation can be arbitrarily fast, for example by doing nothing. Therefore one first needs to specify an accuracy goal and then select the implementation which achieves this in the shortest time. In the case of spiking neuron models we have two measures: the precision of the spike times and the number of missed spikes. The _canon model was included in the code base for NEST not for production but for documenting the results of the publication above and as a reference. When comparing _ps implementations among each other and with grid-constrained methods it also needs to be considered that for modern _ps algorithms the computation time step h can be increased because these models jump from incoming spike to incoming spike anyway.

Later results are summarized in:

Hanuschkin A, Kunkel S, Helias M, Morrison A and Diesmann M (2010) A general and efficient method for incorporating precise spike times in globally time-driven simulations. Front. Neuroinform. 4:113

Krishnan J, Porta Mana P, Helias M, Diesmann M and Di Napoli E (2018) Perfect Detection of Spikes in the Linear Sub-threshold Dynamics of Point Neurons. Front. Neuroinform. 11:75

@github-actions github-actions bot removed the stale Automatic marker for inactivity, please have another look here label Nov 29, 2022
@clinssen clinssen moved this from To do to In progress in Models Nov 29, 2022
@heplesser
Copy link
Contributor

heplesser commented Nov 29, 2022

The figure below shows the median spike time error for 288 spikes recorded over 10 s from a single neuron driven by precise Poisson spike trains mimicking a brunel_ps network with mean rate 28.6 sp/s. Error is computed relative to a reference solution obtained with the iaf_psc_alpha_ps model at resolution $h=2^{-12}$ms. This is similar to the approach of Hanuschkin et al (2010). Errors were obtained for the following models:

  • grid: iaf_psc_alpha
  • can/lin: iaf_psc_alpha_canon with linear interpolation
  • can/cub: iaf_psc_alpha_canon with cubic interpolation
  • ps: iaf_psc_alpha_ps

To achieve the same accuracy obtained with ps for $h=0.5$ms, one needs to run can/cub with $h=2*-9$ms. Then, runtimes are (best of three runs, same machine as above)

Neuron model Simulation time
iaf_psc_alpha_canon (cubic) 73.1 s
iaf_psc_alpha_ps 56.0 s

Accuracy taken into account, iaf_psc_alpha_ps clearly outperforms iaf_psc_alpha_canon and there is no reason to keep iaf_psc_alpha_canon any longer.

spike_time_error

@heplesser heplesser removed the request for review from suku248 November 30, 2022 13:58
@heplesser heplesser changed the title Remove deprecated models Remove deprecated models iaf_psc_alpha_canon and pp_pop_psc_delta Nov 30, 2022
@heplesser heplesser merged commit e899520 into nest:master Nov 30, 2022
@heplesser heplesser moved this from In progress to Done in Models Dec 1, 2022
@heplesser
Copy link
Contributor

For reference, attached is the notebook used to create the plot above.
NEST-PR2294-PrecisionComparison.ipynb.zip

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: Behavior changes Introduces changes that produce different results for some users S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation.
Projects
Models
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants