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

Correct input handling for aeif_cond_beta_multisynapse #510 and Update aeif_cond_beta_multisynapse to new aeif numerics approach #514 #516

Merged
merged 25 commits into from Oct 26, 2016

Conversation

Projects
None yet
3 participants
@golosio
Contributor

golosio commented Oct 7, 2016

I modified input handling for aeif_cond_beta_multisynapse according to the decision made in the NEST Developer VC 19 Sep 2016, see issue #510

golosio added some commits Oct 7, 2016

Added reversal potential array E_rev, with one value of the reversal
potential for each receptor port.
Fixed the way how the number of receptor ports, taus_rise,
taus_decay and E_rev can be updated.
Fixed the pynest example in aeif_cond_beta_multisynapse.h .
@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Oct 7, 2016

Contributor

I also adapted the example in aeif_cond_beta_multisynapse.h and the SLI test.
I still have to do the check that weights are positive and take care of issue #514

Contributor

golosio commented Oct 7, 2016

I also adapted the example in aeif_cond_beta_multisynapse.h and the SLI test.
I still have to do the check that weights are positive and take care of issue #514

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Oct 7, 2016

Contributor

@heplesser about the weight check, should I use a simple assert, as
assert( e.get_delay() > 0 );
on the same line as the other checks that are done in handle( SpikeEvent& e ), or should I throw an exception with an error message, as
throw BadProperty("Synaptic weights for conductance based models must be positive." );

Contributor

golosio commented Oct 7, 2016

@heplesser about the weight check, should I use a simple assert, as
assert( e.get_delay() > 0 );
on the same line as the other checks that are done in handle( SpikeEvent& e ), or should I throw an exception with an error message, as
throw BadProperty("Synaptic weights for conductance based models must be positive." );

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Oct 8, 2016

Contributor

@golosio Raising an exception is certainly more user friendly, so I would suggest that.

Contributor

heplesser commented Oct 8, 2016

@golosio Raising an exception is certainly more user friendly, so I would suggest that.

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Oct 9, 2016

Contributor

Ok, I should have finished. I made a test using the same parameters of the lsodar test, with one receptor port and synaptic rise time = decay time so that the models behaves as the alpha model, and the output is the same as that of aeif_cond_alpha, see the scripts:
aeif_ms.py.txt
aeif_DT0_ms.py.txt
@heplesser and @Silmathoron I wait for your revision.

Contributor

golosio commented Oct 9, 2016

Ok, I should have finished. I made a test using the same parameters of the lsodar test, with one receptor port and synaptic rise time = decay time so that the models behaves as the alpha model, and the output is the same as that of aeif_cond_alpha, see the scripts:
aeif_ms.py.txt
aeif_DT0_ms.py.txt
@heplesser and @Silmathoron I wait for your revision.

@golosio golosio changed the title from Correct input handling for aeif_cond_beta_multisynapse #510 to Correct input handling for aeif_cond_beta_multisynapse #510 and Update aeif_cond_beta_multisynapse to new aeif numerics approach #514 Oct 9, 2016

Show outdated Hide outdated models/aeif_cond_beta_multisynapse.cpp
"The neuron has connections, therefore the number of ports cannot be "
"reduced." );
"The reversal potential, synaptic rise time and synaptic decay time "
"arrays must be modified altogether." );

This comment has been minimized.

@Silmathoron

Silmathoron Oct 9, 2016

Contributor

I don't think "altogether" is the appropriate term here...
Beyond that, I think what we want to check is only change in length... users should be able to modify only one of the properties as long as they change the values inside and not the number of values, right?

@Silmathoron

Silmathoron Oct 9, 2016

Contributor

I don't think "altogether" is the appropriate term here...
Beyond that, I think what we want to check is only change in length... users should be able to modify only one of the properties as long as they change the values inside and not the number of values, right?

This comment has been minimized.

@golosio

golosio Oct 10, 2016

Contributor

It's a problem with my English, I will fix the error message.... About the implementation of the constraints, I followed @heplesser suggestions in comment #510 (comment) when he says:
"if a call to SetStatus contains taus_syn, it also must contain E_rev, and they must be of the same length". In case of aeif_cond_alpha_multisynapse there is no taus_syn, but taus_rise and taus_decay, so I guess that SetStatus should either contain none of them or all three...
If I understand well, what you are proposing is that you can also call SetStatus with one or two of them as far as they have the same length that they had previously. For me it would be fine. @heplesser what do you think about this?

@golosio

golosio Oct 10, 2016

Contributor

It's a problem with my English, I will fix the error message.... About the implementation of the constraints, I followed @heplesser suggestions in comment #510 (comment) when he says:
"if a call to SetStatus contains taus_syn, it also must contain E_rev, and they must be of the same length". In case of aeif_cond_alpha_multisynapse there is no taus_syn, but taus_rise and taus_decay, so I guess that SetStatus should either contain none of them or all three...
If I understand well, what you are proposing is that you can also call SetStatus with one or two of them as far as they have the same length that they had previously. For me it would be fine. @heplesser what do you think about this?

This comment has been minimized.

@heplesser

heplesser Oct 13, 2016

Contributor

@golosio @Silmathoron Apologies for being imprecise in my previous comment. I indeed meant what @Silmathoron suggested: As long as you do not change the number of synapses, you can change the values in one array alone (e.g. taus_syn), but when you change then number, you have to provide all arrays and they need to have consistent size.

@heplesser

heplesser Oct 13, 2016

Contributor

@golosio @Silmathoron Apologies for being imprecise in my previous comment. I indeed meant what @Silmathoron suggested: As long as you do not change the number of synapses, you can change the values in one array alone (e.g. taus_syn), but when you change then number, you have to provide all arrays and they need to have consistent size.

This comment has been minimized.

@golosio

golosio Oct 14, 2016

Contributor

I changed the error message. Please check if it is ok now, otherwise please suggest a suitable one. I modified the behavior for changes in the arrays according to @Silmathoron indication.

@golosio

golosio Oct 14, 2016

Contributor

I changed the error message. Please check if it is ok now, otherwise please suggest a suitable one. I modified the behavior for changes in the arrays according to @Silmathoron indication.

@heplesser

@golosio The code is fine overall, but there are a number of details to fix and improvements possible that should be in place before merging.

Show outdated Hide outdated models/aeif_cond_beta_multisynapse.cpp
@@ -20,6 +20,8 @@
*
*/
#include <stdio.h>

This comment has been minimized.

@heplesser

heplesser Oct 13, 2016

Contributor

You should not use C-headers.

@heplesser

heplesser Oct 13, 2016

Contributor

You should not use C-headers.

This comment has been minimized.

@golosio

golosio Oct 14, 2016

Contributor

Ops... sorry I use it for myself for debugging and I forgot it. Removed.

@golosio

golosio Oct 14, 2016

Contributor

Ops... sorry I use it for myself for debugging and I forgot it. Removed.

Show outdated Hide outdated models/aeif_cond_beta_multisynapse.cpp
@@ -41,6 +43,7 @@
#include "doubledatum.h"
#include "integerdatum.h"
using namespace std;

This comment has been minimized.

@heplesser

heplesser Oct 13, 2016

Contributor

References to elements of the std namespace should always be explicit, never use using namespace std in NEST.

@heplesser

heplesser Oct 13, 2016

Contributor

References to elements of the std namespace should always be explicit, never use using namespace std in NEST.

This comment has been minimized.

@golosio

golosio Oct 14, 2016

Contributor

removed

@golosio

golosio Oct 14, 2016

Contributor

removed

Show outdated Hide outdated models/aeif_cond_beta_multisynapse.cpp
"The neuron has connections, therefore the number of ports cannot be "
"reduced." );
"The reversal potential, synaptic rise time and synaptic decay time "
"arrays must be modified altogether." );

This comment has been minimized.

@heplesser

heplesser Oct 13, 2016

Contributor

@golosio @Silmathoron Apologies for being imprecise in my previous comment. I indeed meant what @Silmathoron suggested: As long as you do not change the number of synapses, you can change the values in one array alone (e.g. taus_syn), but when you change then number, you have to provide all arrays and they need to have consistent size.

@heplesser

heplesser Oct 13, 2016

Contributor

@golosio @Silmathoron Apologies for being imprecise in my previous comment. I indeed meant what @Silmathoron suggested: As long as you do not change the number of synapses, you can change the values in one array alone (e.g. taus_syn), but when you change then number, you have to provide all arrays and they need to have consistent size.

Show outdated Hide outdated models/aeif_cond_beta_multisynapse.cpp
, has_connections_( false )
{
static const double E_rev_def = 0.0; // E_rev default value
static const double taus_rise_def = 2.0; // taus_rise default value
static const double taus_decay_def = 20.0; // taus_decay default value

This comment has been minimized.

@heplesser

heplesser Oct 13, 2016

Contributor

Why do you have these constants here? You could initialize the vectors in the initializer list as

taus_rise(1, 2.0)

But we should still wait for the discussion about default values in the next Developer VC.

@heplesser

heplesser Oct 13, 2016

Contributor

Why do you have these constants here? You could initialize the vectors in the initializer list as

taus_rise(1, 2.0)

But we should still wait for the discussion about default values in the next Developer VC.

This comment has been minimized.

@golosio

golosio Oct 14, 2016

Contributor

good, now the code is much more compact, there is only the initializer list and the body is empty. For now I'm still using the default with one receptor port, but it will be easy to remove it.

@golosio

golosio Oct 14, 2016

Contributor

good, now the code is much more compact, there is only the initializer list and the body is empty. For now I'm still using the default with one receptor port, but it will be easy to remove it.

Show outdated Hide outdated models/aeif_cond_beta_multisynapse.cpp
static const double taus_rise_def = 2.0; // taus_rise default value
static const double taus_decay_def = 20.0; // taus_decay default value
receptor_types.clear();
E_rev.clear();
taus_rise.clear();
taus_decay.clear();

This comment has been minimized.

@heplesser

heplesser Oct 13, 2016

Contributor

No need to clear here, we are in a constructor!

@heplesser

heplesser Oct 13, 2016

Contributor

No need to clear here, we are in a constructor!

This comment has been minimized.

@golosio

golosio Oct 14, 2016

Contributor

removed.

@golosio

golosio Oct 14, 2016

Contributor

removed.

Show outdated Hide outdated models/aeif_cond_beta_multisynapse.h
voltmeter = nest.Create('voltmeter', 1, {'withgid': True})
delays=[1.0, 300.0, 500.0, 700.0]
w=[1.0, 1.0, 1.0, -1.0]
w=[1.0, 1.0, 1.0, 1.0]
for syn in range(4):
nest.Connect(spike, neuron, syn_spec={'model': 'static_synapse',
'weight': w[syn],

This comment has been minimized.

@heplesser

heplesser Oct 13, 2016

Contributor

Don't you need to specify receptor_type here?

@heplesser

heplesser Oct 13, 2016

Contributor

Don't you need to specify receptor_type here?

This comment has been minimized.

@golosio

golosio Oct 14, 2016

Contributor

You mean that the order should be important? I do not know, I did not see any difference in the simulations if I put 'receptor_type' at the end of the syn_spec list, in any case now I've put it in the second position after 'model'.

@golosio

golosio Oct 14, 2016

Contributor

You mean that the order should be important? I do not know, I did not see any difference in the simulations if I put 'receptor_type' at the end of the syn_spec list, in any case now I've put it in the second position after 'model'.

Show outdated Hide outdated testsuite/unittests/test_aeif_cond_beta_multisynapse.sli
/V_peak 0. def
/a 4. def
/b 80.5 def
/V_peak 0. def

This comment has been minimized.

@heplesser

heplesser Oct 13, 2016

Contributor

Fix whitespace here and in the following lines.

@heplesser

heplesser Oct 13, 2016

Contributor

Fix whitespace here and in the following lines.

This comment has been minimized.

@golosio

golosio Oct 14, 2016

Contributor

done

@golosio

golosio Oct 14, 2016

Contributor

done

Show outdated Hide outdated testsuite/unittests/test_aeif_cond_beta_multisynapse.sli
/V_peak 0. def
/a 4. def
/b 80.5 def
/num_of_receptors 3 def

This comment has been minimized.

@heplesser

heplesser Oct 13, 2016

Contributor

Should not be set explicitly.

@heplesser

heplesser Oct 13, 2016

Contributor

Should not be set explicitly.

This comment has been minimized.

@golosio

golosio Oct 14, 2016

Contributor

removed

@golosio

golosio Oct 14, 2016

Contributor

removed

Show outdated Hide outdated testsuite/unittests/test_aeif_cond_beta_multisynapse.sli
/taus_decay [400.0 200.0 200.0] def % synaptic decay times
/taus_rise [200.0 100.0 100.0] def % synaptic rise times
/weights [0.1 0.1 -0.1] def % synaptic weights
/weights [0.1 0.1 0.1] def % synaptic weights

This comment has been minimized.

@heplesser

heplesser Oct 13, 2016

Contributor

Wouldn't it be better to make all reversal potentials, time constants and weights different from each other to make sure nothing gets mixed up in the code?

@heplesser

heplesser Oct 13, 2016

Contributor

Wouldn't it be better to make all reversal potentials, time constants and weights different from each other to make sure nothing gets mixed up in the code?

This comment has been minimized.

@golosio

golosio Oct 14, 2016

Contributor

ok I used values different from each other.

@golosio

golosio Oct 14, 2016

Contributor

ok I used values different from each other.

Show outdated Hide outdated testsuite/unittests/test_aeif_cond_beta_multisynapse.sli
/taus_decay [400.0 200.0 200.0] def % synaptic decay times
/taus_rise [200.0 100.0 100.0] def % synaptic rise times
/weights [0.1 0.1 -0.1] def % synaptic weights
/weights [0.1 0.1 0.1] def % synaptic weights
/delays [1. 1000. 1500.] def % ms - synaptic delays

This comment has been minimized.

@heplesser

heplesser Oct 13, 2016

Contributor

Do you need such long delays? Shorter simulation times would make the test run faster.

@heplesser

heplesser Oct 13, 2016

Contributor

Do you need such long delays? Shorter simulation times would make the test run faster.

This comment has been minimized.

@golosio

golosio Oct 14, 2016

Contributor

ok I increased g_L so the approximation used for the analytical calculation is also valid for smaller synaptic times, and I could also decrease the tolerance. I could decrease the simulation time and now the test is 10 times faster.

@golosio

golosio Oct 14, 2016

Contributor

ok I increased g_L so the approximation used for the analytical calculation is also valid for smaller synaptic times, and I could also decrease the tolerance. I could decrease the simulation time and now the test is 10 times faster.

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Oct 14, 2016

Contributor

@heplesser @Silmathoron ok I made all the changes that you requested.

Contributor

golosio commented Oct 14, 2016

@heplesser @Silmathoron ok I made all the changes that you requested.

@heplesser

@golosio Thank you for your changes. I added some more small suggestions that will make the code a bit more compact, I believe.

It is a bit unusual that State_::set() allows setting of conductances and their derivatives. We don't support that for other models and I am a bit sceptical about adding it here, especially the derivative.

Show outdated Hide outdated models/aeif_cond_beta_multisynapse.h
// boolean flag which indicates whether the neuron has connections
bool has_connections_;
Parameters_(); //!< Sets default parameter values
void get( DictionaryDatum& ) const; //!< Store current values in dictionary
void set( const DictionaryDatum& ); //!< Set values from dictionary
inline size_t

This comment has been minimized.

@heplesser

heplesser Oct 15, 2016

Contributor

Could you add a doxygen comment describing the method, and add an empty line before it?

@heplesser

heplesser Oct 15, 2016

Contributor

Could you add a doxygen comment describing the method, and add an empty line before it?

This comment has been minimized.

@golosio

golosio Oct 15, 2016

Contributor

done

@golosio

golosio Oct 15, 2016

Contributor

done

Show outdated Hide outdated models/aeif_cond_beta_multisynapse.cpp
}
V_.g0_.resize( P_.num_of_receptors );
V_.g0_.resize( P_.n_receptors() );

This comment has been minimized.

@heplesser

heplesser Oct 15, 2016

Contributor

I think it would be good for readability to point out that g0_ will be initialized in the loop below.

@heplesser

heplesser Oct 15, 2016

Contributor

I think it would be good for readability to point out that g0_ will be initialized in the loop below.

This comment has been minimized.

@golosio

golosio Oct 15, 2016

Contributor

done

@golosio

golosio Oct 15, 2016

Contributor

done

Show outdated Hide outdated models/aeif_cond_beta_multisynapse.cpp
S_.y_.resize( State_::NUMBER_OF_FIXED_STATES_ELEMENTS
+ ( State_::NUMBER_OF_STATES_ELEMENTS_PER_RECEPTOR
* P_.num_of_receptors ) );
+ ( State_::NUMBER_OF_STATES_ELEMENTS_PER_RECEPTOR * P_.n_receptors() ) );

This comment has been minimized.

@heplesser

heplesser Oct 15, 2016

Contributor

I think it would be good to explicitly give the initial value 0 as second argument to resize().

@heplesser

heplesser Oct 15, 2016

Contributor

I think it would be good to explicitly give the initial value 0 as second argument to resize().

Show outdated Hide outdated models/aeif_cond_beta_multisynapse.cpp
@@ -168,60 +156,54 @@ aeif_cond_beta_multisynapse::Parameters_::set( const DictionaryDatum& d )
updateValue< double >( d, names::C_m, C_m );
updateValue< double >( d, names::g_L, g_L );
std::vector< double > Erev_tmp, taur_tmp, taud_tmp;
size_t old_n_receptors = n_receptors();

This comment has been minimized.

@heplesser

heplesser Oct 15, 2016

Contributor

Should be const.

@heplesser

heplesser Oct 15, 2016

Contributor

Should be const.

This comment has been minimized.

@golosio

golosio Oct 15, 2016

Contributor

done

@golosio

golosio Oct 15, 2016

Contributor

done

Show outdated Hide outdated models/aeif_cond_beta_multisynapse.cpp
bool taud_flag =
updateValue< std::vector< double > >( d, names::taus_decay, taud_tmp );
updateValue< std::vector< double > >( d, names::taus_decay, taus_decay );
if ( Erev_flag || taur_flag || taud_flag )

This comment has been minimized.

@heplesser

heplesser Oct 15, 2016

Contributor

I think it should be possible to simplify this some more:

// n_receptors() == E_rev.size()
if ( n_receptors() != taus_rise.size() || n_receptors() != taus_decay.size() )
{
  throw BadProperty("E_rev, taus_raus and taus_decays must have the same size.");
}

Now we know all is consistent I would then check n_receptors() != old_n_receptors && has_connections and n_receptors > 0. Then the check that the taus > 0 and tau_rise < tau_decay. That should maybe be protected by if ( taur_flag || taud_flag ) since we have loops here and would want to avoid running through them if the taus have not been changed.

@heplesser

heplesser Oct 15, 2016

Contributor

I think it should be possible to simplify this some more:

// n_receptors() == E_rev.size()
if ( n_receptors() != taus_rise.size() || n_receptors() != taus_decay.size() )
{
  throw BadProperty("E_rev, taus_raus and taus_decays must have the same size.");
}

Now we know all is consistent I would then check n_receptors() != old_n_receptors && has_connections and n_receptors > 0. Then the check that the taus > 0 and tau_rise < tau_decay. That should maybe be protected by if ( taur_flag || taud_flag ) since we have loops here and would want to avoid running through them if the taus have not been changed.

This comment has been minimized.

@golosio

golosio Oct 15, 2016

Contributor

I agree that the code can be made more compact if we simply want to check consistency. However my idea is that we should not only check for consistency, but also give the user a feedback that suggests him how he should modify his command. For instance, if the current number of receptor ports is 4 (or if they have not yet been initialized so the number is 0 or 1) and the user types a command as:
nest.SetStatus(neuron, {'taus_decay':[50.0,20.0,20.0]})
I think a message as:
"If the number of receptor ports is changed, all three arrays E_rev, taus_rise and taus_decay must be provided."
is more helpful to the user than:
"E_rev, taus_rise and taus_decay must have the same size."
because with the latter message it may not be obvious to all users that the problem can be solved by providing all three arrays as arguments.

@golosio

golosio Oct 15, 2016

Contributor

I agree that the code can be made more compact if we simply want to check consistency. However my idea is that we should not only check for consistency, but also give the user a feedback that suggests him how he should modify his command. For instance, if the current number of receptor ports is 4 (or if they have not yet been initialized so the number is 0 or 1) and the user types a command as:
nest.SetStatus(neuron, {'taus_decay':[50.0,20.0,20.0]})
I think a message as:
"If the number of receptor ports is changed, all three arrays E_rev, taus_rise and taus_decay must be provided."
is more helpful to the user than:
"E_rev, taus_rise and taus_decay must have the same size."
because with the latter message it may not be obvious to all users that the problem can be solved by providing all three arrays as arguments.

Show outdated Hide outdated models/aeif_cond_beta_multisynapse.cpp
{
throw BadProperty( "The neuron must have at least one port." );
}
if ( taur_tmp.size() < num_of_receptors && has_connections_ == true )
if ( taus_rise.size() < old_n_receptors && has_connections_ == true )

This comment has been minimized.

@heplesser

heplesser Oct 15, 2016

Contributor

Avoid comparisons with true and false.

@heplesser

heplesser Oct 15, 2016

Contributor

Avoid comparisons with true and false.

This comment has been minimized.

@golosio

golosio Oct 15, 2016

Contributor

done

@golosio

golosio Oct 15, 2016

Contributor

done

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Oct 15, 2016

Contributor

Ok I'll implement the changes that you suggest. Just one note and a question. When you say

It is a bit unusual that State_::set() allows setting of conductances and their derivatives. We don't support that for other models and I am a bit sceptical about adding it here, especially the derivative.

Actually this is how it is done in aeif_cond_alpha_multisynapse, which was the base for this model; I do not know about other models. Where do you suggest to move this part of the code?

Contributor

golosio commented Oct 15, 2016

Ok I'll implement the changes that you suggest. Just one note and a question. When you say

It is a bit unusual that State_::set() allows setting of conductances and their derivatives. We don't support that for other models and I am a bit sceptical about adding it here, especially the derivative.

Actually this is how it is done in aeif_cond_alpha_multisynapse, which was the base for this model; I do not know about other models. Where do you suggest to move this part of the code?

@Silmathoron

This comment has been minimized.

Show comment
Hide comment
@Silmathoron

Silmathoron Oct 15, 2016

Contributor

@golosio I think what @heplesser means is that maybe the user should not be able to modify these variables.
I must say that I do not really agree on that, but I guess we should discuss it on Monday (actually I'm waiting for the VC before I start my review, which is why I haven't been very active lately; I hope all these issues will be clearer afterwards).
In the meantime, I guess you should probably leave that as it is ;)

Contributor

Silmathoron commented Oct 15, 2016

@golosio I think what @heplesser means is that maybe the user should not be able to modify these variables.
I must say that I do not really agree on that, but I guess we should discuss it on Monday (actually I'm waiting for the VC before I start my review, which is why I haven't been very active lately; I hope all these issues will be clearer afterwards).
In the meantime, I guess you should probably leave that as it is ;)

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Oct 15, 2016

Contributor

@heplesser I made the changes that you suggested, except for the two for which I wait for your feedback. I also wait for @Silmathoron review after the VC.

Contributor

golosio commented Oct 15, 2016

@heplesser I made the changes that you suggested, except for the two for which I wait for your feedback. I also wait for @Silmathoron review after the VC.

@Silmathoron

This comment has been minimized.

Show comment
Hide comment
@Silmathoron

Silmathoron Oct 17, 2016

Contributor

Ok, the VC concluded that default behaviour should be that of the single synapse model, so you should implement a default with 1 synapse and the same parameters as the aeif_cond_alpha, except eventually for the synaptic time constants... I'll let you judge on that unless @heplesser has specific suggestions.

As for the dynamics functions, the VC agreed on removing the _DT0 so you can update the function according to your suggestion and @heplesser's comment. This also means (I think), that function and calibration can go back to Buffer and init_buffers_().

Eventually, I think the people in the VC mostly thought giving access to all state variables in set was rather uninteresting but no one seemed stronlgy opposed to it either... so my guess is you can do as you feel like, but maybe @heplesser would like to comment on that...

Contributor

Silmathoron commented Oct 17, 2016

Ok, the VC concluded that default behaviour should be that of the single synapse model, so you should implement a default with 1 synapse and the same parameters as the aeif_cond_alpha, except eventually for the synaptic time constants... I'll let you judge on that unless @heplesser has specific suggestions.

As for the dynamics functions, the VC agreed on removing the _DT0 so you can update the function according to your suggestion and @heplesser's comment. This also means (I think), that function and calibration can go back to Buffer and init_buffers_().

Eventually, I think the people in the VC mostly thought giving access to all state variables in set was rather uninteresting but no one seemed stronlgy opposed to it either... so my guess is you can do as you feel like, but maybe @heplesser would like to comment on that...

@Silmathoron

Ok, here comes the review!

There are also a few problems that were missed previously on lines 303 and 310 so I can't comment on them directly anymore, but they should be corrected:
unless I'm mistaken, dg is the derivative, so error message on L303 should be "Conductances and derivatives arrays should have the same size", and on L310, dg should be allowed to be negative.

Show outdated Hide outdated models/aeif_cond_beta_multisynapse.cpp
}
throw BadProperty(
"The reversal potential, synaptic rise time and synaptic decay time "
"arrays must have the same size." );

This comment has been minimized.

@Silmathoron

Silmathoron Oct 17, 2016

Contributor

Are both tests really necessary?
Wouldn't

bool size_changed = ( E_rev.size() != old_n_receptors
  || taus_rise.size() != old_n_receptors
  || taus_decay.size() != old_n_receptors );
bool change_is_consistent = ( E_rev.size() == taus_rise.size() )
  && ( E_rev.size() == taus_decay.size() );

if ( size_changed && !change_is_consistent )

be sufficient?

@Silmathoron

Silmathoron Oct 17, 2016

Contributor

Are both tests really necessary?
Wouldn't

bool size_changed = ( E_rev.size() != old_n_receptors
  || taus_rise.size() != old_n_receptors
  || taus_decay.size() != old_n_receptors );
bool change_is_consistent = ( E_rev.size() == taus_rise.size() )
  && ( E_rev.size() == taus_decay.size() );

if ( size_changed && !change_is_consistent )

be sufficient?

This comment has been minimized.

@golosio

golosio Oct 18, 2016

Contributor

About this point, please have a look at my answer in
#516 (comment)
I agree that if we simply want to check that the changes are consistent, the code can be made more compact. However I think that we should also think about what error messages would be more helpful for the user.
I think we should distinguish those two erroneous situations:

  1. The user types a command that provides only one or two arrays, and they have a different size from the old one. Then the error message should be something like:
    "If the number of receptor ports is changed, all three arrays E_rev, taus_rise and taus_decay must be provided."
  2. The user types a command that provides three arrays, but they have inconsistent sizes
    Then the error message should be something like:
    "The reversal potential, synaptic rise time and synaptic decay time arrays must have the same size."
    If we first agree on which error messages should be printed in different situations, then we can discuss on what is the best way to implement it in the code.
@golosio

golosio Oct 18, 2016

Contributor

About this point, please have a look at my answer in
#516 (comment)
I agree that if we simply want to check that the changes are consistent, the code can be made more compact. However I think that we should also think about what error messages would be more helpful for the user.
I think we should distinguish those two erroneous situations:

  1. The user types a command that provides only one or two arrays, and they have a different size from the old one. Then the error message should be something like:
    "If the number of receptor ports is changed, all three arrays E_rev, taus_rise and taus_decay must be provided."
  2. The user types a command that provides three arrays, but they have inconsistent sizes
    Then the error message should be something like:
    "The reversal potential, synaptic rise time and synaptic decay time arrays must have the same size."
    If we first agree on which error messages should be printed in different situations, then we can discuss on what is the best way to implement it in the code.

This comment has been minimized.

@Silmathoron

Silmathoron Oct 18, 2016

Contributor

My bad, I remember reading about something like what you said. Ok, I understand your point.

@Silmathoron

Silmathoron Oct 18, 2016

Contributor

My bad, I remember reading about something like what you said. Ok, I understand your point.

Show outdated Hide outdated models/aeif_cond_beta_multisynapse.cpp
@@ -233,14 +215,29 @@ aeif_cond_beta_multisynapse::Parameters_::set( const DictionaryDatum& d )
updateValue< double >( d, names::gsl_error_tol, gsl_error_tol );
if ( V_peak_ <= V_th )
if ( V_reset_ >= V_peak_ )

This comment has been minimized.

@Silmathoron

Silmathoron Oct 17, 2016

Contributor

I think the if ( V_peak_ <= V_th ) test just disappeared... and it shouldn't have ^^

@Silmathoron

Silmathoron Oct 17, 2016

Contributor

I think the if ( V_peak_ <= V_th ) test just disappeared... and it shouldn't have ^^

This comment has been minimized.

@golosio

golosio Oct 18, 2016

Contributor

I copied this part from aeif_cond_alpha, where this check was also missing... should we put it back in both this model and in aeif_cond_alpha?

@golosio

golosio Oct 18, 2016

Contributor

I copied this part from aeif_cond_alpha, where this check was also missing... should we put it back in both this model and in aeif_cond_alpha?

This comment has been minimized.

@Silmathoron

Silmathoron Oct 18, 2016

Contributor

Ok, then this is probably my fault ^^"
No, just change it here, I'll correct the aeif_cond_alpha it in the PR suppressing the dynamics_DT0

@Silmathoron

Silmathoron Oct 18, 2016

Contributor

Ok, then this is probably my fault ^^"
No, just change it here, I'll correct the aeif_cond_alpha it in the PR suppressing the dynamics_DT0

Show outdated Hide outdated models/aeif_cond_beta_multisynapse.cpp
// set the right threshold depending on Delta_T
if ( P_.Delta_T > 0. )
{
B_.V_peak = P_.V_peak_;

This comment has been minimized.

@Silmathoron

Silmathoron Oct 17, 2016

Contributor

I'm almost sure this is supposed to be in calibrate since we want this check to be done every time Simulate is called, so V_peak should be in V_; @heplesser, could you confirm?

@Silmathoron

Silmathoron Oct 17, 2016

Contributor

I'm almost sure this is supposed to be in calibrate since we want this check to be done every time Simulate is called, so V_peak should be in V_; @heplesser, could you confirm?

This comment has been minimized.

@golosio

golosio Oct 18, 2016

Contributor

done

@golosio

golosio Oct 18, 2016

Contributor

done

Show outdated Hide outdated models/aeif_cond_beta_multisynapse.cpp
@@ -656,46 +662,30 @@ aeif_cond_beta_multisynapse_dynamics( double,
// good compiler will optimize the verbosity away ...
// shorthand for state variables
const double& V = y[ S::V_M ];
const double& V = std::min( y[ S::V_M ], node.P_.V_peak_ ); // bound V

This comment has been minimized.

@Silmathoron

Silmathoron Oct 17, 2016

Contributor

Maybe we should add a comment to assure people that we indeed want to use P_.V_peak_ and not V_.V_peak here...

@Silmathoron

Silmathoron Oct 17, 2016

Contributor

Maybe we should add a comment to assure people that we indeed want to use P_.V_peak_ and not V_.V_peak here...

This comment has been minimized.

@golosio

golosio Oct 18, 2016

Contributor

done. However when I will commit please check if this is sufficient or please suggest a more suitable comment.

@golosio

golosio Oct 18, 2016

Contributor

done. However when I will commit please check if this is sufficient or please suggest a more suitable comment.

Show outdated Hide outdated models/aeif_cond_beta_multisynapse.cpp
@@ -563,7 +563,7 @@ aeif_cond_beta_multisynapse::update( Time const& origin,
{
S_.y_[ State_::V_M ] = P_.V_reset_;
}
else if ( S_.y_[ State_::V_M ] >= P_.V_peak_ )
else if ( S_.y_[ State_::V_M ] >= B_.V_peak )

This comment has been minimized.

@Silmathoron

Silmathoron Oct 17, 2016

Contributor

Switch V_peak to V_ (see above)

@Silmathoron

Silmathoron Oct 17, 2016

Contributor

Switch V_peak to V_ (see above)

This comment has been minimized.

@golosio

golosio Oct 18, 2016

Contributor

done

@golosio

golosio Oct 18, 2016

Contributor

done

Show outdated Hide outdated models/aeif_cond_beta_multisynapse.cpp
// If the argument is too large, we clip it.
const double I_spike =
node.P_.Delta_T * std::exp( std::min( exp_arg, MAX_EXP_ARG ) );
const double I_spike = node.P_.Delta_T == 0. ? 0 : node.P_.Delta_T

This comment has been minimized.

@Silmathoron

Silmathoron Oct 17, 2016

Contributor

To be consistent (and homogeneous), change to node.P_.Delta_T * node.P_.g_L * std::exp( ( V - node.P_.V_th ) / node.P_.Delta_T )

@Silmathoron

Silmathoron Oct 17, 2016

Contributor

To be consistent (and homogeneous), change to node.P_.Delta_T * node.P_.g_L * std::exp( ( V - node.P_.V_th ) / node.P_.Delta_T )

This comment has been minimized.

@golosio

golosio Oct 18, 2016

Contributor

As a physicist I like when variables have a name appropriate for what they represent and consistent with their units!

@golosio

golosio Oct 18, 2016

Contributor

As a physicist I like when variables have a name appropriate for what they represent and consistent with their units!

This comment has been minimized.

@heplesser

heplesser Oct 24, 2016

Contributor

I think for clarity, it would be good to place everything after the : in parentheses, even if not strictly necessary.

@heplesser

heplesser Oct 24, 2016

Contributor

I think for clarity, it would be good to place everything after the : in parentheses, even if not strictly necessary.

Show outdated Hide outdated models/aeif_cond_beta_multisynapse.cpp
// dv/dt
f[ S::V_M ] = ( -node.P_.g_L * ( ( V - node.P_.E_L ) - I_spike ) + I_syn - w
f[ S::V_M ] = ( -node.P_.g_L * ( V - node.P_.E_L - I_spike ) + I_syn - w

This comment has been minimized.

@Silmathoron

Silmathoron Oct 17, 2016

Contributor

Switch to ( -node.P_.g_L * ( V - node.P_.E_L ) + I_spike + I_syn + ...

@Silmathoron

Silmathoron Oct 17, 2016

Contributor

Switch to ( -node.P_.g_L * ( V - node.P_.E_L ) + I_spike + I_syn + ...

This comment has been minimized.

@golosio

golosio Oct 18, 2016

Contributor

done

@golosio

golosio Oct 18, 2016

Contributor

done

Show outdated Hide outdated models/aeif_cond_beta_multisynapse.h
@@ -294,6 +298,7 @@ class aeif_cond_beta_multisynapse : public Archiving_Node
* the first simulation, but not modified before later Simulate calls.
*/
double I_stim_;
double V_peak;

This comment has been minimized.

@Silmathoron

Silmathoron Oct 17, 2016

Contributor

Should be in V_ because it can be modified before Simulate

@Silmathoron

Silmathoron Oct 17, 2016

Contributor

Should be in V_ because it can be modified before Simulate

This comment has been minimized.

@golosio

golosio Oct 18, 2016

Contributor

done

@golosio

golosio Oct 18, 2016

Contributor

done

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Oct 18, 2016

Contributor

Thank you @Silmathoron , I implemented the default with one synapse and the same parameters as the aeif_cond_alpha, except for the synaptic rise time and decay time. I updated the dynamic function according to @heplesser suggestion.
About State_::set() allowing to set conductances and their derivatives, I think @heplesser already said his opinion, so I would remove this possibility, unless you think there is really a reason to allow it

Contributor

golosio commented Oct 18, 2016

Thank you @Silmathoron , I implemented the default with one synapse and the same parameters as the aeif_cond_alpha, except for the synaptic rise time and decay time. I updated the dynamic function according to @heplesser suggestion.
About State_::set() allowing to set conductances and their derivatives, I think @heplesser already said his opinion, so I would remove this possibility, unless you think there is really a reason to allow it

@Silmathoron

This comment has been minimized.

Show comment
Hide comment
@Silmathoron

Silmathoron Oct 18, 2016

Contributor

I would rather say that I don't really see a reason to remove it if you implemented it ^^
Otherwise I don't really care since I don't have a use case for the moment... it's up to you, as far as I am concerned.

Contributor

Silmathoron commented Oct 18, 2016

I would rather say that I don't really see a reason to remove it if you implemented it ^^
Otherwise I don't really care since I don't have a use case for the moment... it's up to you, as far as I am concerned.

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Oct 18, 2016

Contributor

@Silmathoron
ok since @heplesser was skeptic about it I will remove it.

unless I'm mistaken, dg is the derivative, so error message on L303 should be "Conductances and derivatives arrays should have the same size", and on L310, dg should be allowed to be negative.

As I will remove this part of the code, those changes are not necessary any more. However, note that what is called dg in the code IS NOT the derivative of g. If you look for instance Roth and van Rossum, "Modeling synapses"
http://homepages.inf.ed.ac.uk/mvanross/reprints/roth_mvr_chap.pdf
equations 6.7 - 6.9 and compare them with the equations in aeif_cond_beta_multisynapse_dynamic you can observe that what is called dg (or y[ S::DG + j ] ) in the code is actually the function that is called h in those equations, and from equation 6.8 you can see that the derivative dg/dt is not h, because there is another term. Sorry, but the code of aeif_cond_beta_multisynapse was originally based on aeif_cond_alpha_multisynapse, and I did not modify the name of the variable dg although I thought it was somehow inappropriate.

Contributor

golosio commented Oct 18, 2016

@Silmathoron
ok since @heplesser was skeptic about it I will remove it.

unless I'm mistaken, dg is the derivative, so error message on L303 should be "Conductances and derivatives arrays should have the same size", and on L310, dg should be allowed to be negative.

As I will remove this part of the code, those changes are not necessary any more. However, note that what is called dg in the code IS NOT the derivative of g. If you look for instance Roth and van Rossum, "Modeling synapses"
http://homepages.inf.ed.ac.uk/mvanross/reprints/roth_mvr_chap.pdf
equations 6.7 - 6.9 and compare them with the equations in aeif_cond_beta_multisynapse_dynamic you can observe that what is called dg (or y[ S::DG + j ] ) in the code is actually the function that is called h in those equations, and from equation 6.8 you can see that the derivative dg/dt is not h, because there is another term. Sorry, but the code of aeif_cond_beta_multisynapse was originally based on aeif_cond_alpha_multisynapse, and I did not modify the name of the variable dg although I thought it was somehow inappropriate.

@Silmathoron

This comment has been minimized.

Show comment
Hide comment
@Silmathoron

Silmathoron Oct 18, 2016

Contributor

Ok, I did not see that S::DG and dg were different; this is indeed not optimal and speaks in favour of removing it.
By the way, I just noticed that the docstring of the model in aeif_cond_beta_multisynapse.h does not include a description of the parameters, so could you add one? (see example in aeif_cond_alpha)

Contributor

Silmathoron commented Oct 18, 2016

Ok, I did not see that S::DG and dg were different; this is indeed not optimal and speaks in favour of removing it.
By the way, I just noticed that the docstring of the model in aeif_cond_beta_multisynapse.h does not include a description of the parameters, so could you add one? (see example in aeif_cond_alpha)

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Oct 18, 2016

Contributor

the docstring of the model in aeif_cond_beta_multisynapse.h does not include a description of the parameters, so could you add one? (see example in aeif_cond_alpha

sure, thank you for indicating the example!

Contributor

golosio commented Oct 18, 2016

the docstring of the model in aeif_cond_beta_multisynapse.h does not include a description of the parameters, so could you add one? (see example in aeif_cond_alpha

sure, thank you for indicating the example!

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Oct 18, 2016

Contributor

@Silmathoron Thank you for your review. I think I'm done in making the changes that you requested.

Contributor

golosio commented Oct 18, 2016

@Silmathoron Thank you for your review. I think I'm done in making the changes that you requested.

Show outdated Hide outdated models/aeif_cond_beta_multisynapse.h
@@ -64,6 +64,48 @@
reversal potentials are supplied by the array "E_rev". The port numbers
are automatically assigned in the range from 1 to n_receptors.
During connection, the ports are selected with the property "receptor_type".
The membrane potential is given by the following differential equation:
C dV/dt= -g_L(V-E_L)+g_L*Delta_T*exp((V-V_T)/Delta_T)-g_e(t)(V-E_e)
-g_i(t)(V-E_i)-w +I_e

This comment has been minimized.

@Silmathoron

Silmathoron Oct 18, 2016

Contributor

Ok, sorry, one last comment: since it's multisynapse, here, you should probably detail:

C dV/dt= -g_L(V-E_L) + g_L*Delta_T*exp((V-V_T)/Delta_T) + I_syn_tot(V, t) - w + I_e

where:
I_syn_tot(V,t) = \sum_i g_i(t) (V - E_{rev,i})

and the synapse i is excitatory or inhibitory depending on the value of E_{rev,i}.
@Silmathoron

Silmathoron Oct 18, 2016

Contributor

Ok, sorry, one last comment: since it's multisynapse, here, you should probably detail:

C dV/dt= -g_L(V-E_L) + g_L*Delta_T*exp((V-V_T)/Delta_T) + I_syn_tot(V, t) - w + I_e

where:
I_syn_tot(V,t) = \sum_i g_i(t) (V - E_{rev,i})

and the synapse i is excitatory or inhibitory depending on the value of E_{rev,i}.
@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Oct 18, 2016

Contributor

@Silmathoron done!

Contributor

golosio commented Oct 18, 2016

@Silmathoron done!

@Silmathoron

All good for me 👍

@heplesser

@golosio Looks good, except for a few small details. Thanks for your efforts!

Show outdated Hide outdated nestkernel/nest_names.h
@@ -407,6 +407,8 @@ extern const Name taus_eta; //!< Specific to population point process model
extern const Name taus_syn; //!< Synapse time constants (array)
extern const Name taus_rise; //!< Synapse rise constants (array)
extern const Name taus_decay; //!< Synapse decay constants (array)
extern const Name E_rev; //!< Reversal potential (array)

This comment has been minimized.

@heplesser

heplesser Oct 24, 2016

Contributor

Entries in nest_names.[h,cpp] should be in strictly alphabetical order.

@heplesser

heplesser Oct 24, 2016

Contributor

Entries in nest_names.[h,cpp] should be in strictly alphabetical order.

Show outdated Hide outdated nestkernel/nest_names.h
@@ -407,6 +407,8 @@ extern const Name taus_eta; //!< Specific to population point process model
extern const Name taus_syn; //!< Synapse time constants (array)
extern const Name taus_rise; //!< Synapse rise constants (array)
extern const Name taus_decay; //!< Synapse decay constants (array)
extern const Name E_rev; //!< Reversal potential (array)
extern const Name num_of_receptors; //!< number of receptor ports

This comment has been minimized.

@heplesser

heplesser Oct 24, 2016

Contributor

In other names, we have used n_, e.g., n_events. I suggest n_receptors here for consistency.

@heplesser

heplesser Oct 24, 2016

Contributor

In other names, we have used n_, e.g., n_events. I suggest n_receptors here for consistency.

Show outdated Hide outdated models/aeif_cond_beta_multisynapse.cpp
S_.y_.resize( State_::NUMBER_OF_FIXED_STATES_ELEMENTS
+ ( State_::NUMBER_OF_STATES_ELEMENTS_PER_RECEPTOR
* P_.num_of_receptors_ ) );
+ ( State_::NUMBER_OF_STATES_ELEMENTS_PER_RECEPTOR * P_.n_receptors() ) );

This comment has been minimized.

@heplesser

heplesser Oct 24, 2016

Contributor

I'd suggest explicitly initializing with 0.0 here, as second argument.

@heplesser

heplesser Oct 24, 2016

Contributor

I'd suggest explicitly initializing with 0.0 here, as second argument.

Show outdated Hide outdated models/aeif_cond_beta_multisynapse.cpp
// If the argument is too large, we clip it.
const double I_spike =
node.P_.Delta_T * std::exp( std::min( exp_arg, MAX_EXP_ARG ) );
const double I_spike = node.P_.Delta_T == 0. ? 0 : node.P_.Delta_T

This comment has been minimized.

@heplesser

heplesser Oct 24, 2016

Contributor

I think for clarity, it would be good to place everything after the : in parentheses, even if not strictly necessary.

@heplesser

heplesser Oct 24, 2016

Contributor

I think for clarity, it would be good to place everything after the : in parentheses, even if not strictly necessary.

@Silmathoron

This comment has been minimized.

Show comment
Hide comment
@Silmathoron

Silmathoron Oct 25, 2016

Contributor

By the way, @golosio, I think you also forgot to readd the V_peak_ > V_th check in your branch... I was looking for it in your code to add it again in the psc models x)

Contributor

Silmathoron commented Oct 25, 2016

By the way, @golosio, I think you also forgot to readd the V_peak_ > V_th check in your branch... I was looking for it in your code to add it again in the psc models x)

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Oct 25, 2016

Contributor

@Silmathoron maybe you are looking to an updated version of the file... it should be in line 218.

Contributor

golosio commented Oct 25, 2016

@Silmathoron maybe you are looking to an updated version of the file... it should be in line 218.

@Silmathoron

This comment has been minimized.

Show comment
Hide comment
@Silmathoron

Silmathoron Oct 25, 2016

Contributor

My bad, you're probably right, must have been an outdated file.
All good!

Contributor

Silmathoron commented Oct 25, 2016

My bad, you're probably right, must have been an outdated file.
All good!

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Oct 25, 2016

Contributor

@golosio As in #513 , I would suggest we allow V_th == V_peak and rephrase the error message.

Contributor

heplesser commented Oct 25, 2016

@golosio As in #513 , I would suggest we allow V_th == V_peak and rephrase the error message.

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Oct 25, 2016

Contributor

@heplesser done!

Contributor

golosio commented Oct 25, 2016

@heplesser done!

@heplesser

@golosio Just a little bit of clean-up left in the user documentation comment :).

Show outdated Hide outdated models/aeif_cond_beta_multisynapse.h
rise time and decay time constant. Synaptic conductance is modeled by a
aeif_cond_beta_multisynapse is a conductance based adaptive exponential
integrate-and-fire neuron model. It allows an arbitrary number of synaptic
rise time and decay time constants. Synaptic conductance is modeled by a
beta function, as described by A. Roth and M.C.W. van Rossum
in Computational Modeling Methods for Neuroscientists, MIT Press 2013,
Chapter 6

This comment has been minimized.

@heplesser

heplesser Oct 26, 2016

Contributor

Fullstop missing at end of sentence.

@heplesser

heplesser Oct 26, 2016

Contributor

Fullstop missing at end of sentence.

This comment has been minimized.

@golosio

golosio Oct 26, 2016

Contributor

done.

@golosio

golosio Oct 26, 2016

Contributor

done.

Show outdated Hide outdated models/aeif_cond_beta_multisynapse.h
are then automatically assigned and there range is from 1 to n. (n
being the index of the last element of the tau_ex and tau_in
arrays).
The number of receptor ports is supplied by the function "n_receptors".

This comment has been minimized.

@heplesser

heplesser Oct 26, 2016

Contributor

I would drop this sentence, and n_receptors is not a function. I think the meaning of n_receptors becomes clear from the last sentence in this paragraph.

@heplesser

heplesser Oct 26, 2016

Contributor

I would drop this sentence, and n_receptors is not a function. I think the meaning of n_receptors becomes clear from the last sentence in this paragraph.

This comment has been minimized.

@golosio

golosio Oct 26, 2016

Contributor

done.

@golosio

golosio Oct 26, 2016

Contributor

done.

Show outdated Hide outdated models/aeif_cond_beta_multisynapse.h
and the synapse i is excitatory or inhibitory depending on the value of
E_{rev,i}.

This comment has been minimized.

@heplesser

heplesser Oct 26, 2016

Contributor

The differential equation for w fell out in one of your previous commits. I think it should be here. Would you add it again?

@heplesser

heplesser Oct 26, 2016

Contributor

The differential equation for w fell out in one of your previous commits. I think it should be here. Would you add it again?

This comment has been minimized.

@golosio

golosio Oct 26, 2016

Contributor

done.

@golosio

golosio Oct 26, 2016

Contributor

done.

Show outdated Hide outdated models/aeif_cond_beta_multisynapse.h
gsl_error_tol double - This parameter controls the admissible error of the
GSL integrator. Reduce it if NEST complains about
numerical instabilities.
Examples:
% PyNEST example, of how to assign synaptic rise time and decay time
% to a receptor type.

This comment has been minimized.

@heplesser

heplesser Oct 26, 2016

Contributor

I think the comment here is not needed. If you want to keep it, mark the comment with Python's # instead of %, which introduces comments in SLI.

@heplesser

heplesser Oct 26, 2016

Contributor

I think the comment here is not needed. If you want to keep it, mark the comment with Python's # instead of %, which introduces comments in SLI.

This comment has been minimized.

@golosio

golosio Oct 26, 2016

Contributor

removed.

@golosio

golosio Oct 26, 2016

Contributor

removed.

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Oct 26, 2016

Contributor

@heplesser ok I made the changes that you requested!

Contributor

golosio commented Oct 26, 2016

@heplesser ok I made the changes that you requested!

@heplesser

@golosio All well now!

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Oct 26, 2016

Contributor

@golosio Thank you for your efforts! Together with @Silmathoron's "all good" above, I will merge.

Contributor

heplesser commented Oct 26, 2016

@golosio Thank you for your efforts! Together with @Silmathoron's "all good" above, I will merge.

@heplesser heplesser merged commit b1cc2c8 into nest:master Oct 26, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@golosio golosio deleted the golosio:reversal_potential branch Oct 26, 2016

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Oct 26, 2016

Contributor

Thank you @heplesser and @Silmathoron !

Contributor

golosio commented Oct 26, 2016

Thank you @heplesser and @Silmathoron !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment