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

Moved check_synapse_params to specific synapse classes #739

Merged
merged 6 commits into from Jun 6, 2017

Conversation

@stinebuu
Copy link
Contributor

stinebuu commented May 31, 2017

This PR fixes #737 .

It takes each test in ConnBuilder::check_synapse_params() and moves it to a check_synapse_params() function in the respective synapse class. To make sure we don't have to add it to all the synapse classes, the GenericConnectionModel handles the allocation.

A regressiontest is also added, which make sure all tests fail if we send in the specific parameters in the syn_spec dictionary.

@heplesser
Copy link
Contributor

heplesser commented May 31, 2017

@stinebuu All subtest in issue-737.sli that use threading should go into a separate file and be skipped if threading is not available.

@heplesser heplesser self-requested a review Jun 1, 2017
@heplesser
Copy link
Contributor

heplesser commented Jun 1, 2017

@kappeld Could you review this pull request? Since it adds the method check_synapse_params() to classes ConnectorModel and Connection, it may have an effect on your non-generic connector model. I particular, I think your connector model will have to implement the method.

Copy link
Contributor

heplesser left a comment

Looks pretty good, I have just som comments that mainly concern documentation.

@@ -141,6 +141,12 @@ class ContDelayConnection : public Connection< targetidentifierT >
void set_status( const DictionaryDatum& d, ConnectorModel& cm );

/**
* Check syn_spec dictionary for parameters that are not allowed for this
* connection. Will issue warning or throw error if a parameter is found.

This comment has been minimized.

@heplesser

heplesser Jun 1, 2017 Contributor

  • this general explanation should be in the base class only (Connection)
  • here, you could just mention what is illegal for this particular synapse model
  • Doxygen comments should begin with a single-line summary followed by details
  • "an illegal parameter is found"
  • add info that the method does nothing otherwise
@@ -127,6 +127,12 @@ class Quantal_StpConnection : public Connection< targetidentifierT >
void set_status( const DictionaryDatum& d, ConnectorModel& cm );

/**
* Check syn_spec dictionary for parameters that are not allowed for this
* connection. Will issue warning or throw error if a parameter is found.

This comment has been minimized.

@heplesser

heplesser Jun 1, 2017 Contributor

See above.

@@ -143,6 +143,21 @@ class StaticConnectionHomW : public Connection< targetidentifierT >
}

/**
* Check syn_spec dictionary for parameters that are not allowed for this
* connection. Will issue warning or throw error if a parameter is found.
*/

This comment has been minimized.

@heplesser

heplesser Jun 1, 2017 Contributor

See above.

@@ -197,6 +197,12 @@ class STDPDopaConnection : public Connection< targetidentifierT >
void set_status( const DictionaryDatum& d, ConnectorModel& cm );

/**
* Check syn_spec dictionary for parameters that are not allowed for this
* connection. Will issue warning or throw error if a parameter is found.
*/

This comment has been minimized.

@heplesser

heplesser Jun 1, 2017 Contributor

See above.

synapse_model_ = kernel().model_manager.get_synapsedict()->lookup( syn_name );

// We need to make sure that Connect can process all synapse parameters
// specified.
ConnectorModel& syn_mod =

This comment has been minimized.

@heplesser

heplesser Jun 1, 2017 Contributor

This could be const, and would suggest the following renaming:

  • synapse_model_ -> synapse_model_id_ (because it just is the id number)
  • syn_mod-> synapse_model
@@ -150,6 +150,12 @@ class Connection
void set_status( const DictionaryDatum& d, ConnectorModel& cm );

/**
* Check syn_spec dictionary for parameters that are not allowed for this
* connection. Will issue warning or throw error if a parameter is found.
*/

This comment has been minimized.

@heplesser

heplesser Jun 1, 2017 Contributor

See above, the base class doc should be here. Mention that the base class implementation does nothing and that classes requiring checks need to override with their own method.

@@ -150,6 +150,8 @@ class ConnectorModel

virtual const CommonSynapseProperties& get_common_properties() const = 0;

virtual void check_synapse_params( const DictionaryDatum& ) const = 0;

This comment has been minimized.

@heplesser

heplesser Jun 1, 2017 Contributor

Add documentation.


skip_if_not_threaded

% Test multithreded c in syn_spec for stdp_dopamine_synapse model

This comment has been minimized.

@heplesser

heplesser Jun 1, 2017 Contributor

"a" missing in "multithreaded"; I think that is in other places, too.

syn_mod to synapse_model in conn_builder.
@kappeld
Copy link
Contributor

kappeld commented Jun 1, 2017

At a first glance the changes look good to me, but they will effect our custom connector model as @heplesser suggested. I will try this out and review the changes more carefully as soon as possible.

Copy link
Contributor

heplesser left a comment

Looks good, just a few more details on comments.

@@ -141,8 +141,7 @@ class ContDelayConnection : public Connection< targetidentifierT >
void set_status( const DictionaryDatum& d, ConnectorModel& cm );

/**
* Check syn_spec dictionary for parameters that are not allowed for this
* connection. Will issue warning or throw error if a parameter is found.
* Checks to see if delay is given in syn_spec.

This comment has been minimized.

@heplesser

heplesser Jun 1, 2017 Contributor

This is a bit confusing. I would rather write Throws exception if delays is given. Otherwise, the reader may think that the check will throw and exception if the weight is missing.

@@ -127,8 +127,7 @@ class Quantal_StpConnection : public Connection< targetidentifierT >
void set_status( const DictionaryDatum& d, ConnectorModel& cm );

/**
* Check syn_spec dictionary for parameters that are not allowed for this
* connection. Will issue warning or throw error if a parameter is found.
* Checks to see if n or a are given in syn_spec.

This comment has been minimized.

@heplesser

heplesser Jun 1, 2017 Contributor

Also here change the wording so that it is clear that n and a are "prohibited".

* Checks to see if illegal parameters are given in syn_spec.
*
* The illegal parameters are: vt, A_minus, A_plus, Wmax, Wmin, b, tau_c,
* tau_n, tau_plus, c and n. The last two only apply if we have more than one

This comment has been minimized.

@heplesser

heplesser Jun 1, 2017 Contributor

"only apply" -> "are prohibited only"

@heplesser heplesser requested a review from jakobj Jun 2, 2017
@jakobj
jakobj approved these changes Jun 2, 2017
Copy link
Contributor

jakobj left a comment

Great work! Much clearer now. I just have this one comment below, otherwise I am happy.

* @note Classes requiring checks need to override the function with their own
* implementation, as this base class implementation does not do anything.
*/
void check_synapse_params( const DictionaryDatum& d ) const;

This comment has been minimized.

@jakobj

jakobj Jun 2, 2017 Contributor

If subclasses should override this function, shouldn't it be declared virtual?

This comment has been minimized.

@stinebuu

stinebuu Jun 2, 2017 Author Contributor

@jakobj As far as I understand, the connection class is not supposed to have any virtual functions, because we then store pointers to the subclasses so we can use the correct overridden function. By making this into an empty function, we avoid this memory allocation.

@heplesser does this sound about right?

This comment has been minimized.

@heplesser

heplesser Jun 2, 2017 Contributor

Yes, the technical term is vtable: For the 4G kernel we worked hard to avoid virtual functions in Connection objects so that these objects do not contain a vtable-pointer, which would cost 8B per synapse; see Kunkel et al (2014) for details.

This comment has been minimized.

@jakobj

jakobj Jun 2, 2017 Contributor

Ah alright, should've known that. ;) thanks for reminding me.

@heplesser
Copy link
Contributor

heplesser commented Jun 2, 2017

Before merging, wait for feedback from @kappeld concerning the effect on module implementing its own ConnectorModel.

@kappeld
kappeld approved these changes Jun 6, 2017
Copy link
Contributor

kappeld left a comment

I have checked out these changes and ran our tests. The modified API works fine with our custom nest module.

@kappeld
Copy link
Contributor

kappeld commented Jun 6, 2017

On a related note: Since the API of NEST has changed now quite a bit since the 2.12.0 release (with this and other changes), @heplesser are you planning to issue a 2.12.1 release soon? We would like to keep the version numbers of our custom module in sync with NEST's version number. Since the changes made to NEST also require a slightly updated version of our module to be compatible, making a new minor release might be more transparent for the users.

@heplesser
Copy link
Contributor

heplesser commented Jun 6, 2017

@kappeld Thanks for checking! Yes, we are considering a 2.12.1 release since there have been a number of bug fixes (and a few more under review).

@heplesser
Copy link
Contributor

heplesser commented Jun 6, 2017

Release Notes: Refactored check_synapse_param() to better object-oriented design. Connection subclasses can now define their own checks.

@heplesser heplesser merged commit 277bb37 into nest:master Jun 6, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@stinebuu stinebuu deleted the stinebuu:issue737 branch Jun 21, 2017
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.

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