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

testsuite::issue-521 - *_hpc connections return local id instead of g… #714

Merged
merged 27 commits into from Jun 21, 2017

Conversation

@tillschumann
Copy link
Contributor

@tillschumann tillschumann commented Apr 24, 2017

#521
testsuite::issue-521 - *_hpc connections return local id instead of global id of target neuron

*_hpc connections should only return thread_local_target instead of target
to avoid confusion

Currently, it is not possible to get the target gid from a *_hpc connection.
To get it, we would need to pass the thread-id to the get_status function as an argument.
I think it is easier to leave out "target" as an return entry and return "thread_local_target" instead to avoid confusion.

Copy link
Contributor

@heplesser heplesser left a comment

This looks quite fine, just a few small things to fix.

@@ -322,6 +322,7 @@ const Name t_ref_remaining( "t_ref_remaining" );
const Name t_ref_tot( "t_ref_tot" );
const Name t_spike( "t_spike" );
const Name target( "target" );
const Name thread_local_target( "thread_local_target" );

This comment has been minimized.

@heplesser

heplesser May 8, 2017
Contributor

Name definitions should be strictly alphabetical, please move this down.

@@ -377,8 +377,9 @@ extern const Name
extern const Name t_ref_tot; //!< Total refractory period, iaf_tum_2000
extern const Name t_spike; //!< Time of last spike
extern const Name target; //!< Connection parameters
extern const Name target_thread; //!< Connection parameters
extern const Name targets; //!< Connection parameters
extern const Name thread_local_target; //!< Connection parameters

This comment has been minimized.

@heplesser

heplesser May 8, 2017
Contributor

Please move this down so the order remains strictly alphabetical.

ResetKernel
0 << /local_num_threads threads >> SetStatus

/iaf_neuron 20 Create

This comment has been minimized.

@heplesser

heplesser May 8, 2017
Contributor

iaf_neuron will be deprecated soon, please use iaf_psc_alpha or similar.


13 GetStatus /target_dict Set

5 13 /static_synapse Connect

This comment has been minimized.

@heplesser

heplesser May 8, 2017
Contributor

Is there any special significance to using neurons 5 and 13? If so, it would be good to explain it briefly.

Copy link
Contributor

@suku248 suku248 left a comment

I understand that it would be quite inconvenient to change the signature of set_status. However, I am not quite happy with the solution here. It would be more consistent to provide the target gid also for *_hpc synapses. Please have a look at this code.

Copy link
Contributor

@heplesser heplesser left a comment

@tillschumann Just two little things left from my side; a recent improvement in unittest requires a small adaptation.


M_ERROR setverbosity

is_threaded_with_openmp not { exit_test_gracefully } if

This comment has been minimized.

@heplesser

heplesser Jun 1, 2017
Contributor

Please replace this line with

skip_if_not_threaded

which has been added to master a few days ago. Travis currently fails without threading because we changed the behavior of exit_test_gracefully behind the scenes.



% create set of neurons
/iaf_neuron 20 Create

This comment has been minimized.

@heplesser

heplesser Jun 1, 2017
Contributor

Please use /iaf_psca_alpha, /iaf_neuron is deprecated.

@heplesser
Copy link
Contributor

@heplesser heplesser commented Jun 1, 2017

@kappeld This bug fix PR changes the signature of ConnectorBase::get_synapse_status(). Could you check if that will cause problems for your module and report back here?

Copy link
Contributor

@kappeld kappeld left a comment

Tests passed! No conflicts from our side!

Copy link
Contributor

@suku248 suku248 left a comment

👍

Updated test description to the actual solution of this issue.
@heplesser
Copy link
Contributor

@heplesser heplesser commented Jun 21, 2017

Release nodes: GetConnections work correctly now also for _hpc synapses.

@heplesser heplesser merged commit f201ca0 into nest:master Jun 21, 2017
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
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

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