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

Ensure Name insertion is done in thread-serial context and replace strings with Name objects #724

Merged
merged 13 commits into from May 22, 2017

Conversation

@stinebuu
Copy link
Contributor

stinebuu commented May 19, 2017

This PR fixes #722 and #719 by

  • Adding an assert to make sure Name::insert() is only executed in thread-serial context.
  • Replacing strings in get_status() and set_status() dictionary lookups to Name objects.
Copy link
Contributor

heplesser left a comment

This looks very fine, just some minor points on alphabetical order. A proper regression test for this issue would be difficult to create, since the Name table is created only once per nest invocation and not reset by ResetKernel. Since we are playing it safe now with the assertion in Name::insert() and had some other tests that triggered when the Name table got corrupted, I think we can proceed without an explicit unit test here.

*/
namespace names
{

This comment has been minimized.

@heplesser

heplesser May 19, 2017 Contributor

Remove spurious empty line.

sli/name.cc Outdated
@@ -78,6 +80,9 @@ Name::insert( const std::string& s )

if ( where == map.end() )
{
#ifdef _OPENMP
assert( not omp_in_parallel() );

This comment has been minimized.

@heplesser

heplesser May 19, 2017 Contributor

It would be useful to add a comment here pointing out why this assertion is in place: to protect the global name table. We do not protect by pragma omp critical since that could lead to hard-to-find performance problems due to serialization.

sli/name.cc Outdated
#include <iomanip>
#include <iostream>
#include <omp.h>

This comment has been minimized.

@heplesser

heplesser May 19, 2017 Contributor

This needs to be protected by an #ifdef _OPENMP for systems that do not have OpenMP available.

extern const Name a_acausal; //!< Used by stdp_connection_facetshw_hom
extern const Name a_causal; //!< Used by stdp_connection_facetshw_hom
extern const Name a_thresh_th; //!< Used by stdp_connection_facetshw_hom
extern const Name a_thresh_tl; //!< Used by stdp_connection_facetshw_hom

This comment has been minimized.

@heplesser

heplesser May 19, 2017 Contributor

Place all the a_... names further up with the A_... in alphabetical order of the ... part.

extern const Name alpha; //!< stdp_synapse parameter
extern const Name alpha_1; //!< Specific to Kobayashi, Tsubo, Shinomoto 2009
extern const Name alpha_2; //!< Specific to Kobayashi, Tsubo, Shinomoto 2009
extern const Name Aminus; //!< Used by stdp_connection_facetshw_hom

This comment has been minimized.

@heplesser

heplesser May 19, 2017 Contributor

Here we get a problem that nest_names was meant to avoid: we introduce A_minus and Aminus. But we cannot fix this here, that is something for NEST3.


extern const Name d; //!< Specific to Izhikevich 2003
extern const Name D_lower;
extern const Name D_mean;
extern const Name D_std;
extern const Name D_upper;
extern const Name data_path; //!< Data path, used by io_manager
extern const Name data_prefix; //!< Data prefix, used by io_manager
extern const Name data; //!< Used in music_message_in_proxy

This comment has been minimized.

@heplesser

heplesser May 19, 2017 Contributor

Put this before the data_... entries.

@@ -457,6 +560,7 @@ extern const Name
V_peak; //!< Spike detection threshold (Brette & Gerstner 2005)
extern const Name V_reset; //!< Reset potential
extern const Name V_T_star; //!< Specific to gif models
extern const Name V_T; //!< Voltage offset

This comment has been minimized.

@heplesser

heplesser May 19, 2017 Contributor

Should be before V_T_star.

Copy link
Contributor

heplesser left a comment

Approved conditional to Travis passing.

@heplesser heplesser requested a review from gtrensch May 19, 2017
Copy link
Collaborator

gtrensch left a comment

This looks ok to me !

@jougs
jougs approved these changes May 22, 2017
Copy link
Contributor

jougs left a comment

Nice works! Many thanks for fixing.

@jougs jougs merged commit 15ac89b into nest:master May 22, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@stinebuu stinebuu deleted the stinebuu:convert_to_names branch May 30, 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.

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