Fix out of bound access on pp_pop_psc_delta and pulsepacket_generator

[niltonlk]

This fix part of #2101

Installcheck with -D_GLIBCXX_ASSERTIONS flag reported out of bound access in pp_pop_psc_delta and pulsepacket_generator (see #2101)

-pp_pop_psc_delta case:
calibration in pp_pop_psc_delta created rhos_ages_ vector 1 unit larger than eta_kernel_ vector.
This caused theta_ages_ vector to be created 1 unit smaller than rhos_ages_ vector in line 348.
This causes out of bound access of theta_ages_ vector in the loop in line 361.

-pulsepacket_generator case:
line 223 was accessing the first element of an empty deque (spiketimes_) due to pop_front in line 221.
This was fixed by checking if spiketimes_ is not empty first (same as line 217)

[heplesser]

This looks like a very brutal fix. I looked some more at the pp_pop_psc_delta code and noticed (a) that the code is in poor shape and (b) that it has carried since Nov 2016 a deprecation notice referring users to gif_pop_psc_exp. I would therefore suggest to remove the model entirely. @jougs @terhorstd What do you think?

[jougs]

I suggest to remove the model in 3.2 and add a corresponding sentence to the detailed release notes of 3.1.

[jougs]

Just to clarify: please add the deprecation info for the model to the release notes in this PR, and open a new issue with the 3.2 milestone set about the removal of the model.

[heplesser]

I agree with @jougs. After some more code-staring, I now also understand why the -1 is correct above. Even though we will remove the model in 3.2, could you add a comment above like

# Set all except last element to zero.

[niltonlk]

Taking a look at the code in gif_pop_psc_exp, another way to fix out of bound access here would be:

    for ( int j = 0; j < V_.len_eta_; j++ )
    {
      S_.age_occupations_.push_back( 0 );
      S_.thetas_ages_.push_back( 0 );
      S_.n_spikes_ages_.push_back( 0 );
      S_.rhos_ages_.push_back( 0 );
    }
    S_.age_occupations_[ V_.len_eta_ - 1 ] = P_.N_;

I just don't know what would be the best comment for this code.

[heplesser]

Given that the code is soon disappearing, I'd keep changes minimal, so your -1 solution is fine. Just put this comment before the loop:

# Set all except last state vector elements to zero, then fill last element with initial value

[niltonlk]

Got it.

[heplesser]

Please replace && with and according to our current style guide. I also think that the extra () are not needed since we have a sequence of ands.

[niltonlk]

Have replaced && with and
There were also some || which was replaced with or
pp_pop_psc_delta was also corrected, although it might get deleted.

[heplesser]

@niltonlk Interestingly, after your sensible fix, test_pulsepacket_generator.sli now fails.

[niltonlk]

The conditional expression to deliver the spike event was wrong. The last spike was never sent as it would not pass the if clause in line 223. Sorry for the silly mistake.

[heplesser]

The conditional expression to deliver the spike event was wrong. The last spike was never sent as it would not pass the if clause in line 223. Sorry for the silly mistake.

Good catch! I had to stare at the code for quite some time to understand how spike emission works in this generator.

[heplesser]

@niltonlk Great, thanks. I created a separate PR for the release notes (#2160), so I will approve this one now.

[niltonlk]

Added deprecation message following 'iaf_psc_alpha_canon'.

My question is, is it really necessary to have deprecation message defined in pynest/nest/lib/hl_api_helper.py as well? Since in pynest the message defined in models/modelsmodule.cpp is also displayed?
Maybe register_node_model in modelsmodule.cpp could follow the same strategy as in hl_api_helper.py to also display the replacement model, and use only this latter method?

[jougs]

LGTM.

Also the way deprecation information is added, is fine by me.