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

Numerical imprecision stdp #924

Merged
merged 6 commits into from Apr 11, 2018
Merged

Conversation

@suku248
Copy link
Contributor

@suku248 suku248 commented Apr 9, 2018

This PR is a temporary fix for issue #894

Copy link
Contributor

@heplesser heplesser left a comment

@suku248 Looks good, I have mostly comments on code formatting and one comment that confuses me. How difficult would it be to create a test for this issue?

@@ -452,7 +452,8 @@ STDPDopaConnection< targetidentifierT >::process_dopa_spikes_(
// process dopa spikes in (t0, t1]
// propagate weight from t0 to t1
if ( ( dopa_spikes.size() > dopa_spikes_idx_ + 1 )
&& ( dopa_spikes[ dopa_spikes_idx_ + 1 ].spike_time_ <= t1 ) )
&& ( t1 - dopa_spikes[ dopa_spikes_idx_ + 1 ].spike_time_ > -1.0
* kernel().connection_manager.get_stdp_eps() ) )

This comment has been minimized.

@heplesser

heplesser Apr 9, 2018
Contributor

Could you try to get the -1.0 on the same line as kernel()....? I think that would be good for legibility.

This comment has been minimized.

@suku248

suku248 Apr 11, 2018
Author Contributor

I am afraid the coding style guidelines do not permit this.

@@ -468,7 +469,8 @@ STDPDopaConnection< targetidentifierT >::process_dopa_spikes_(
// process remaining dopa spikes in (t0, t1]
double cd;
while ( ( dopa_spikes.size() > dopa_spikes_idx_ + 1 )
&& ( dopa_spikes[ dopa_spikes_idx_ + 1 ].spike_time_ <= t1 ) )
&& ( t1 - dopa_spikes[ dopa_spikes_idx_ + 1 ].spike_time_ > -1.0
* kernel().connection_manager.get_stdp_eps() ) )

This comment has been minimized.

@heplesser

heplesser Apr 9, 2018
Contributor

Could you try to get the -1.0 on the same line as kernel()....? I think that would be good for legibility.

This comment has been minimized.

@suku248

suku248 Apr 11, 2018
Author Contributor

Again, not possible due to coding style guidelines.

// occur at the same time
// only depression if pre- and postsyn. spike occur at the same time
if ( t_spike - start->t_ > 1.0
* kernel().connection_manager.get_stdp_eps() )

This comment has been minimized.

@heplesser

heplesser Apr 9, 2018
Contributor

I would drop the multiplication with 1.0. I also don't understand the comment: the comment talks about pre- and post-spike being at the same time, but then you facilitate only if there is a non-zero time difference. Your code does the same thing as the original code, so my confusion also applies to the comment in the original code.

This comment has been minimized.

@suku248

suku248 Apr 11, 2018
Author Contributor

I agree and have removed the 1.0.

The comment is misleading as it's about synaptic depression. I have adjusted it such that it explains that facilitation is skipped in case of 0 temporal difference between pre- and post spike.

runner != history_.end() && runner->t_ <= t_first_read;
runner != history_.end()
&& ( t_first_read - runner->t_ > -1.0
* kernel().connection_manager.get_stdp_eps() );

This comment has been minimized.

@heplesser

heplesser Apr 9, 2018
Contributor

Could you try to get the -1.0 on the same line as kernel()....? I think that would be good for legibility.

This comment has been minimized.

@suku248

suku248 Apr 11, 2018
Author Contributor

Again, not possible due to coding style guidelines.

@@ -100,7 +109,8 @@ nest::Archiving_Node::get_K_value( double t )
int i = history_.size() - 1;
while ( i >= 0 )
{
if ( t > history_[ i ].t_ )
if ( t - history_[ i ].t_ > 1.0
* kernel().connection_manager.get_stdp_eps() )

This comment has been minimized.

@heplesser

heplesser Apr 9, 2018
Contributor

Could you try to get the -1.0 on the same line as kernel()....? I think that would be good for legibility.

This comment has been minimized.

@suku248

suku248 Apr 11, 2018
Author Contributor

It's a +1.0 and I have removed it.

while ( ( runner != history_.end() ) && ( runner->t_ <= t2 ) )
while ( ( runner != history_.end() )
&& ( t2 - runner->t_ > -1.0
* kernel().connection_manager.get_stdp_eps() ) )

This comment has been minimized.

@heplesser

heplesser Apr 9, 2018
Contributor

Could you try to get the -1.0 on the same line as kernel()....? I think that would be good for legibility.

This comment has been minimized.

@suku248

suku248 Apr 11, 2018
Author Contributor

Again, not possible due to coding style guidelines.

@@ -126,7 +136,8 @@ nest::Archiving_Node::get_K_values( double t,
int i = history_.size() - 1;
while ( i >= 0 )
{
if ( t > history_[ i ].t_ )
if ( t - history_[ i ].t_ > 1.0
* kernel().connection_manager.get_stdp_eps() )

This comment has been minimized.

@heplesser

heplesser Apr 9, 2018
Contributor

Drop multiplication by 1.0?

This comment has been minimized.

@suku248

suku248 Apr 11, 2018
Author Contributor

Yes, thanks, done.

@@ -419,6 +423,10 @@ class ConnectionManager : public ManagerInterface

//! Capacity growth factor to use beyond the limit
double large_connector_growth_factor_;

//! Maximum distance between (double) spike times in STDP that is
//! still considered 0

This comment has been minimized.

@heplesser

heplesser Apr 9, 2018
Contributor

I think it would be useful to refer to the issue here.

This comment has been minimized.

@suku248

suku248 Apr 11, 2018
Author Contributor

Yes, you are right. Done.

* Set epsilon that is used for comparing spike times in STDP.
* Spike times in STDP synapses are currently represented as double
* values. The epsilon defines the maximum distance between spike
* times that is still considered 0.

This comment has been minimized.

@heplesser

heplesser Apr 9, 2018
Contributor

Maybe add a Note: See issue ...?

This comment has been minimized.

@suku248

suku248 Apr 11, 2018
Author Contributor

Yes, good point. Done.

@@ -1794,6 +1812,9 @@ NestModule::init( SLIInterpreter* i )
"GetStructuralPlasticityStatus", &getstructuralplasticitystatus_function );
i->createcommand( "Disconnect", &disconnect_i_i_lfunction );
i->createcommand( "Disconnect_g_g_D_D", &disconnect_g_g_D_Dfunction );

i->createcommand( "SetStdpEps", &setstdpeps_dfunction );

This comment has been minimized.

@heplesser

heplesser Apr 9, 2018
Contributor

Should we have a GetStdpEps function, too? Or maybe "hide" this in the kernel status dict after all?

This comment has been minimized.

@suku248

suku248 Apr 11, 2018
Author Contributor

No, I don't think that we want a GetStdpEps. As this is a temporary fix, I would not like to make the variable visible in the kernel dict. I have added an info message to SetStdpEps though, which states the new value of stdp_eps and I have added two checks.

@heplesser heplesser added this to the NEST 2.16 milestone Apr 10, 2018
Susanne Kunkel
- Remove unnecessary multiplications with 1.0
- Add checks and info output for SetStdpEps
- Refer to issue #894 in comments
@suku248
Copy link
Contributor Author

@suku248 suku248 commented Apr 11, 2018

Thank you @heplesser for the review!
Please have a look at the new commit.

Copy link
Contributor

@hakonsbm hakonsbm left a comment

@suku248 Looks pretty good to me. I just have a couple comments.

@@ -565,8 +567,9 @@ STDPDopaConnection< targetidentifierT >::send( Event& e,
process_dopa_spikes_( dopa_spikes, t0, start->t_ + dendritic_delay, cp );
t0 = start->t_ + dendritic_delay;
minus_dt = t_last_update_ - t0;
if ( start->t_ < t_spike ) // only depression if pre- and postsyn. spike
// occur at the same time
// faciltate only in case of post- after presyn. spike

This comment has been minimized.

@hakonsbm

hakonsbm Apr 11, 2018
Contributor

Should be "facilitate".

This comment has been minimized.

@suku248

suku248 Apr 11, 2018
Author Contributor

Thanks, fixed!

@@ -58,6 +58,8 @@

// C++ includes:
#include <cmath>
#include <iomanip>
#include <limits>

This comment has been minimized.

@hakonsbm

hakonsbm Apr 11, 2018
Contributor

As far as I can see these aren't needed anywhere.

This comment has been minimized.

@suku248

suku248 Apr 11, 2018
Author Contributor

That's correct. I have removed them.

@@ -29,6 +29,13 @@

#include "archiving_node.h"

// C++ includes:
#include <iomanip>
#include <limits>

This comment has been minimized.

@hakonsbm

hakonsbm Apr 11, 2018
Contributor

As far as I can see these aren't needed anywhere.

This comment has been minimized.

@suku248

suku248 Apr 11, 2018
Author Contributor

That's correct. I have removed them.

@suku248
Copy link
Contributor Author

@suku248 suku248 commented Apr 11, 2018

Thank you @hakonsbm for the comments!

@abigailm abigailm merged commit e5a9cad into nest:master Apr 11, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@aserenko
Copy link
Contributor

@aserenko aserenko commented Apr 26, 2018

@suku248, could you possibly be so kind to have a look at

?

Firstly, there is also a direct comparison to zero that should be corrected in the same way.

The second question is a bit deeper. As far as I understand, now that you have changed nestkernel/archiving_node.cpp, the Archiving_Node::get_history() does not return events with time equal to its start time argument t1. And so, events that would lead to accounting coinciding (zero-delta-t) spike pairs can never be returned by get_history.
That way, the behaviour of this very model that I was wanting to discuss in issue #861 seem to have been changed to the way I argued for in that issue. Do you too think it has?

@suku248
Copy link
Contributor Author

@suku248 suku248 commented Apr 30, 2018

Yes, you are right. I have simply overlooked stdp_connection_facetshw_hom.h.

I am not really familiar with this model. Do you have some experience with the model and do you think you could correct the comparisons of double values for this model and then generate a PR? That would be great.

I think, it's possible that this will solve #861

aserenko added a commit to aserenko/nest-simulator that referenced this pull request May 4, 2018
PR nest#924 fixed direct comparison of delta_t to zero in STDP.
Among other things, that PR added the assertion
  minus_dt < 1.0 * stdp_numerical_imprecision
to all spike-timing-dependent plasticity models. Except
stdp_facetshw synapse, which is fixed in this commit.

In stdp_facetshw synapse the direct comparison to zero was
acceptable, because of the initialization
  double minus_dt = 0;
  double plus_dt = 0;
So, the direct comparison to zero here just checks if `minus_dt`
or `plus_dt` retain their initial values, which basically means
the same as checking `if ( start != finish )`. Still, I remove
the comparison to zero for clarity, since it is redundant.
@aserenko
Copy link
Contributor

@aserenko aserenko commented May 4, 2018

Sorry, mostly a false alarm. There was an initialization a few lines higher:

  double minus_dt = 0;
  double plus_dt = 0;

So, now (as soon as the current PR has been merged) the direct comparison to zero here just checks if minus_dt or plus_dt retain their initial values, which basically means the same as checking if ( start != finish ).

Still, I could not resist the temptation to mess with the code and opened a PR (#952) anyway. However, it is not that necessary, and I won't mind if it is rejected.

aserenko added a commit to aserenko/nest-simulator that referenced this pull request May 6, 2018
PR nest#924 fixed direct comparison of delta_t to zero in STDP.
Among other things, that PR added the assertion
  minus_dt < 1.0 * stdp_numerical_imprecision
to all spike-timing-dependent plasticity models. Except
stdp_facetshw synapse, which is fixed in this commit.

In stdp_facetshw synapse the direct comparison to zero was
acceptable, because of the initialization
  double minus_dt = 0;
  double plus_dt = 0;
So, the direct comparison to zero here just checks if `minus_dt`
or `plus_dt` retain their initial values, which basically means
the same as checking `if ( start != finish )`. Still, I remove
the comparison to zero for clarity, since it is redundant.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.