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

Proposed fix for issue #463 #465

Merged
merged 27 commits into from Oct 17, 2016
Merged

Proposed fix for issue #463 #465

merged 27 commits into from Oct 17, 2016

Conversation

@golosio
Copy link
Contributor

golosio commented Aug 18, 2016

Proposed fix for issue #463
Added possibility to use two-dimensional parameter array in conjunction with connection rules fixed_indegree and fixed_outdegree in Python .
Added Python test for this possibility.

golosio added 3 commits Aug 17, 2016
… the connection rules

fixed_indegree and fixed_outdegree
Added python test for such connections
@golosio golosio changed the title Connect Proposed fix for issue #463 Aug 18, 2016
@golosio
Copy link
Contributor Author

golosio commented Aug 18, 2016

I suggest @heplesser and @jougs as reviewers

@jougs
Copy link
Contributor

jougs commented Aug 19, 2016

Adding @hannahbos as the original author of the array parameter variant. I'm on vacation currently and can only look properly on 29th of August.

@golosio
Copy link
Contributor Author

golosio commented Aug 19, 2016

Thank you @jougs . @hannahbos and @heplesser actually the array parameter variant seems to work properly in nest, without modifications, in conjunction with the connection rules fixed_indegree and fixed_outdegree as well. It is only a pynest check that forbids this possibility from python. But apparently there is no reason to have this restriction, also because the array parameter variant can be very useful. What I did is simply to remove the restriction, put new checks on array dimensions for the fixed in/out degree cases, add a python test.

neuron2 = nest.Create('iaf_neuron', N)

Warr = [[y*10+x for x in range(K)] for y in range(N)]
Darr = [[1.0*y+0.1*x+1.0 for x in range(K)] for y in range(N)]

This comment has been minimized.

Copy link
@heplesser

heplesser Aug 22, 2016

Contributor

You could use integers as well for delays, avoids the need for handling rounding errors.

This comment has been minimized.

Copy link
@golosio

golosio Aug 29, 2016

Author Contributor

Done

nest.Connect(neuron1, neuron2, conn_spec=conn_dict, syn_spec=syn_dict)

w = [0.0, 0.0, 0.0]
d = [0.0, 0.0, 0.0]

This comment has been minimized.

Copy link
@heplesser

heplesser Aug 22, 2016

Contributor

Here you imply that K==3. That should be more flexible.

w1 = w[j]-10.0*i
self.assertTrue(w1 == 0.0 or w1 == 1.0 or w1 == 2.0)

self.assertTrue(w[0] != w[1] and w[0] != w[2] and w[1] != w[2])

This comment has been minimized.

Copy link
@heplesser

heplesser Aug 22, 2016

Contributor

Also here you imply that K == 3.

w[j] = nest.GetStatus(c, 'weight')[0]
d[j] = nest.GetStatus(c, 'delay')[0]
val = d[j]-1.0-w[j]/10.0
self.assertTrue(val < eps and val > -eps)

This comment has been minimized.

Copy link
@heplesser

heplesser Aug 22, 2016

Contributor

Python permits -eps < val < eps or abs(val) < eps.


w = [0.0, 0.0, 0.0]
d = [0.0, 0.0, 0.0]
for i in range(N):

This comment has been minimized.

Copy link
@heplesser

heplesser Aug 22, 2016

Contributor

I wonder if you need the nested loops. Maybe it would be faster to make a single call to GetConnections and GetStatus and then sort things out on the Python level, maybe so that you could directly compare with the original arrays?

@heplesser
Copy link
Contributor

heplesser commented Aug 22, 2016

@golosio We will need to check this carefully. We originally permitted arrays only for one-to-one and all-to-all because we were afraid that the syntax might not be straightforward/intuitive enough and lead to subtle errors (NEST doing something else than the user expects). It appears to me that you logic is that (for fixed in-degree) the outer dimension indexes target neurons and the inner dimension the connections onto each target. That is certainly well-defined. But we should still discuss this in the Developer VC.

There are a number of things to do:

  • Add the new feature to the documentation
  • Make sure we have an mpitest for this feature. This is problematic, since our MPI testing mechanism currently requires SLI and you are implementing this on the Python level. We need to think about this.
  • As a first step, you could extend the Python test to use multiple threads as a first stage of parallelization.
@golosio
Copy link
Contributor Author

golosio commented Aug 26, 2016

@heplesser I'll try to fix the pull request according to all your concerns. I'm also trying to write a sli version of the test, but I was not able to specify an array for weights or delays in the dictionary syn_spec used with the Connect function, not even with the all_to_all rule. Also I can't find an example for this in the documentation. Can you give me some hints?

@golosio
Copy link
Contributor Author

golosio commented Aug 26, 2016

@heplesser ok I think I solved. The weight/delay type in the dictionary is not supposed to be a sli array, but a DoubleVector (or IntVector) so I had to use the sli function cv_dv to convert the sli array to a vector. I had to look in the source code to understand this, as I could not find documentation. . .

golosio added 5 commits Aug 28, 2016
…th the connection rules

fixed_indegree and fixed_outdegree
Improved corresponding pynest test
@jougs
Copy link
Contributor

jougs commented Aug 29, 2016

@golosio: Where in the documentation would you have expected this? I can add the corresponding text if you tell me, where! Thanks!

@golosio
Copy link
Contributor Author

golosio commented Aug 29, 2016

@jougs I think that:

  1. the commands cv_dv and cv_iv should be mentioned in http://www.nest-simulator.org/quickref/
    in the section "Arrays, vectors, and matrices", subsection "conversions"
    and in http://www.nest-simulator.org/objects_and_data_types/ in the section "object types", subsection "Conversion between types".
  2. the types and should be mentioned in http://www.nest-simulator.org/objects_and_data_types/ in the section "arrays" (which could become "arrays and vectors") or next to it. It should be shortly explained why they are needed (maybe for compatibility with some python functions?). Maybe the list in the section "object types" could be expanded to include them...
  3. There is no information about using synapse and connection dictionaries in http://www.nest-simulator.org/neural_simulations/ Just as a starting point, a very short paragraph analogous to that in pynest: http://www.nest-simulator.org/introduction-to-pynest/part-3-connecting-networks-with-synapses/ section "Connecting with synapse models" with a similar example using the corresponding SLI commands, could be very useful.
  4. I would add a section "Connecting with array parameters" with an example similar (but simpler) to the one that I used in test_connect_array.sli
    Let me know what you think and if I can help with the changes that you think are appropriate...
@golosio
Copy link
Contributor Author

golosio commented Aug 29, 2016

@jougs @heplesser if you want I can write a draft of what I would add to the documentation, and then you can review it...

@golosio
Copy link
Contributor Author

golosio commented Aug 29, 2016

@heplesser @jougs As @heplesser suggested I made some test changing the number of virtual processes, and indeed there are problems. For instance if I run this sli script (in nest, also without my changes)
t1.txt
I get wrong results if the number of virtual processes is greater than 1. Next days I'll try to see how difficult is to fix this problem.

… connection rules

fixed_indegree and fixed_outdegree on multiple MPI processes / threads
golosio added 3 commits Aug 31, 2016
…n rule fixed_indegree,

with more than one virtual process
@golosio
Copy link
Contributor Author

golosio commented Aug 31, 2016

@jougs ok, next week I'll try to do it.

@golosio
Copy link
Contributor Author

golosio commented Aug 31, 2016

@heplesser @jougs I wrote two SLI tests for MPI/threads. The test for fixed_outdegree with more than 1 MPI process is more difficult, because connection info are accessible to a MPI process only if the target neuron is associated to that process, thus while in the case of the fixed_indegree rule all the elements of a row of the parameter array can be compared with connection parameters accessible in the same MPI process, for the fixed_outdegree rule this is not the case.
Therefore for this second rule I made a test that works in two levels:

  • a first level checks that for each source neuron the connection parameters accessible to the actual MPI process are a subset of the corresponding row of the parameter array.
  • a comprehensive test that is not run automatically, which could be run only in case of specific problems or major changes. It requires that the first test writes its results (by uncommenting a line in the SLI script) in a set of files, one for each MPI process. Then a bash script joins the files' content in a proper way to check it.
@golosio
Copy link
Contributor Author

golosio commented Aug 31, 2016

@heplesser ok, I made all the changes that you indicated.

  • I made the tests more general, for any values of K
  • I prepared a simple failing test as t1.txt
  • I wrote two SLI tests for MPI/threads.
  • I wrote some documentation in the file hl_api_connections.py
    Only I was not able to avoid a nested loop in test_connect_array.py
    because (e.g. for fixed_indegree with K=3) I want to check that neuron 1 incoming connections have weights 1,2,3 (no matter in which order) neuron 2 have 4,5,6 and so on. Maybe I could do the same building couples (neuron index, connection weight) and sorting at two levels, but I am not so familiar with python and I am not sure it would be simpler.
    I wait for your comments.
@hannahbos
Copy link

hannahbos commented Sep 19, 2016

@golosio. Thank you very much for all the nice work. We excluded these options, because we couldn't come up with a usecase. When it comes to biological networks one should be careful with this option since the resulting network could have a network structure that was not intended. For example, given neuron pairs could be connected with different weights or one neuron could be projecting with a positive weight on some and with a negative weight on other neurons. However, the restriction of the parameter array dimension you implemented as well as the documentation should make the usage straight forward. Below I propose a small change in the wording of the new documentation. Mentioning source neurons or target neurons when describing 'fixed_indegree' and 'fixed_outdegree', respectively, is a bit confusion. I tried to point out that the parameter specification of the indegree/outdegree does not depend on the identity of the source/target neuron. In addition to the docstring, the documentation on the homepage should be updated. Once the documentation issues are resolved I am all in for merging.

For 'fixed_indegree' the array has to be a two-dimensional NumPy array
with shape (len(post), indegree), where indegree is the number of
incoming connections per target neuron, therefore the rows describe the
target and the columns the connections converging to the target neuron,
regardless of the identity of the source neurons.
For 'fixed_outdegree' the array has to be a two-dimensional NumPy array
with shape (len(pre), outdegree), where outdegree is the number of
outgoing connections per source neuron, therefore the rows describe the
source and the columns the connections starting from the source neuron
regardless of the identity of the target neuron.

@golosio
Copy link
Contributor Author

golosio commented Sep 19, 2016

@hannahbos thank you! I will modify the docstring as you suggest and I will update the documentation as soon as possible.

@golosio
Copy link
Contributor Author

golosio commented Sep 24, 2016

@hannahbos ok I changed the docstring as you suggested and I expanded the documentation (file extras/userdoc/md/documentation/connection-management.md) using the same description and including two simple examples.

Copy link
Contributor

heplesser left a comment

@golosio This all looks quite fine now, but I think you can boost performance (see comment above) and polish tests a little.

for ( long j = 0; j < indegree_; ++j )
{
// skip array parameters handled in other virtual processes
skip_conn_parameter_( tid );

This comment has been minimized.

Copy link
@heplesser

heplesser Sep 27, 2016

Contributor

Looping over skip_conn_parameter_() can become very inefficient due to nested loops and function calls, including to ConnParameter::skip(), which requires a vtable lookup. I think it would be far more efficient to pass the number of elements to skip to skip_conn_parameter() as second argument (default value 1), pass that through to ConnParamter::skip() and then use the most efficient implementation "down there". For Array*Parameter::skip(), that probably would just mean replacing *next_[ tid ]++ with next_[ tid ] += j (the derefercing in the current code makes no sense).

This comment has been minimized.

Copy link
@golosio

golosio Oct 12, 2016

Author Contributor

Good idea. I've done it. However I also noticed that the same is true for the connection rule all_to_all
in conn_builder.cpp, line 849-853:

      for ( GIDCollection::const_iterator sgid = sources_->begin();
            sgid != sources_->end();
            ++sgid )
        skip_conn_parameter_( tid );

and same thing in other parts of the same file. Since the number of source neurons is fixed in the all_to_all rule, also in this case it should be possible to avoid those loops and to pass the number of source neurons as second argument of the call to skip_conn_parameter

This comment has been minimized.

Copy link
@heplesser

heplesser Oct 13, 2016

Contributor

@golosio I think that is a good idea, but we should keep PRs focused. Therefore, I would suggest that you create an issue to remind us to refactor the skipping mechanism; then we can address it in a separate PR later.

This comment has been minimized.

Copy link
@golosio

golosio Oct 13, 2016

Author Contributor

ok, I created a new issue.

############################################
# test with connection rule fixed_outdegree
############################################
nest.ResetKernel()

This comment has been minimized.

Copy link
@heplesser

heplesser Sep 27, 2016

Contributor

I would split the test into two here. In that way, test failure will be more specific and thus informative.

This comment has been minimized.

Copy link
@golosio

golosio Oct 12, 2016

Author Contributor

done

NumProcesses /num_mpi_procs Set % gets number of MPI processes

0 <<
/total_num_virtual_procs num_mpi_procs VP_per_MPI mul

This comment has been minimized.

Copy link
@heplesser

heplesser Sep 27, 2016

Contributor

You can also set /local_num_threads 2 directly, then NEST will do the math for you. That is the recommended approach.

This comment has been minimized.

Copy link
@golosio

golosio Oct 12, 2016

Author Contributor

I am not sure of how to do this, since I could not find a suitable example in the mpitests directory. Do you mean that I can delete all the lines:

/VP_per_MPI 2 def % number of virtual processes per MPI process
NumProcesses /num_mpi_procs Set % gets number of MPI processes
0 <<
/total_num_virtual_procs num_mpi_procs VP_per_MPI mul
>> SetStatus % sets number of virtual processes

and replace them with a single instruction

0 <<
/local_num_threads 2
>> SetStatus

?

This comment has been minimized.

Copy link
@heplesser

heplesser Oct 13, 2016

Contributor

@golosio Yes, you can :)!

/w sdict /weight get def % gets synaptic weight
/d sdict /delay get def % gets synaptic delay
/diff d w sub def % diff = delay - weight
diff 1.0 eq assert_or_die % checks that d = w + 1

This comment has been minimized.

Copy link
@heplesser

heplesser Sep 27, 2016

Contributor

You should avoid assert_or_die in MPI tests. Rather, set a boolean flag, e.g., pass here indicating a problem. This also applies to all following assert_or_die. Then, at the very end, have pass n_loc 0 gt and

This comment has been minimized.

Copy link
@golosio

golosio Oct 12, 2016

Author Contributor

done

@@ -0,0 +1,51 @@
#!/bin/bash

This comment has been minimized.

Copy link
@heplesser

heplesser Sep 27, 2016

Contributor

Since this cannot be run automatically, move it to the manualtests directory. With #471 @jougs added a way allowing more complex tests requiring output files. Once that goes "mainstream", we can use it for this test as well.

This comment has been minimized.

Copy link
@golosio

golosio Oct 12, 2016

Author Contributor

done

Warr1 {
/w exch def
% check that all elements of Warr are in Wrow
Wrow w MemberQ assert_or_die

This comment has been minimized.

Copy link
@heplesser

heplesser Sep 27, 2016

Contributor

See comment on fixed-indegree test.

This comment has been minimized.

Copy link
@golosio

golosio Oct 12, 2016

Author Contributor

done


% Define connection and synapse dictionaries
/syn_dict << /model /static_synapse /weight Warr >> def
/conn_dict << /rule /fixed_indegree /indegree 3 >> def

This comment has been minimized.

Copy link
@heplesser

heplesser Sep 27, 2016

Contributor

The indegree is equal to hte set of source neurons. Does that weaken the test? Could you use a larger net1?

This comment has been minimized.

Copy link
@golosio

golosio Oct 12, 2016

Author Contributor

I've increased the number of source neurons(10) target neurons(10) and indegree(5).

@golosio
Copy link
Contributor Author

golosio commented Oct 12, 2016

@heplesser I made all the changes that you requested, except for setting local_num_threads in the sli tests, for which I wait for your help. I also wait that you tell me whether I should also improve the calls to skip_conn_parameters in the rule 'all_to_all'.

@@ -144,7 +144,7 @@ class ConnBuilder
* node is not located on the current thread or MPI-process and read of an
* array.
*/
void skip_conn_parameter_( thread );
void skip_conn_parameter_( thread, int skip_num = 1 );

This comment has been minimized.

Copy link
@heplesser

heplesser Oct 13, 2016

Contributor

Since the number of places to skip cannot be negative, I would suggest size_t as data type. I would also think num_skip or n_skip would be better names, closer to the full "number of positions to skip".

Copy link
Contributor

heplesser left a comment

@golosio It looks very fine now, I just added some tiny suggestions. Once they are fixed I will approve the PR.

@hannahbos Are you happy as well?

@@ -297,10 +297,10 @@ class ArrayIntegerParameter : public ConnParameter
}

void
skip( thread tid ) const
skip( thread tid, int skip_num ) const

This comment has been minimized.

Copy link
@heplesser

heplesser Oct 13, 2016

Contributor

Indentation seems off here?

…nections with parameter arrays
@golosio
Copy link
Contributor Author

golosio commented Oct 13, 2016

@heplesser ok, I made the changes that you requested.

Copy link
Contributor

heplesser left a comment

👍

@golosio
Copy link
Contributor Author

golosio commented Oct 14, 2016

Thank you @heplesser !

@hannahbos
Copy link

hannahbos commented Oct 17, 2016

👍 Thank you again for all the good work!

@golosio
Copy link
Contributor Author

golosio commented Oct 17, 2016

Thank you @hannahbos !

@heplesser heplesser merged commit 87f6034 into nest:master Oct 17, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
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.