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

Refactor Connection Generator Interface #483

Merged
merged 72 commits into from Oct 16, 2017

Conversation

@jougs
Contributor

jougs commented Sep 14, 2016

In particular this PR

  • removes one level of indirection by moving all functionality from the cg_connect.{h,cpp} files to the conngen.{h,cpp} files
  • converts all CGConnect variants to support intvectordatum and gidcollection istead of subnet
  • adds automatic testing of libneurosim to the Travis CI setup
  • fixes and extends the PyNEST test that uses the Python implementation of CSA throu the Connection Generator Interface
  • adds a manual SLI test for the Connection Generator Interface
  • adds more documentation for the Connection Generator Interface

I propose @mdjurfeldt and @uahic as reviewers.

tammoippen and others added some commits Feb 16, 2016

Parse compiler output for number of warnings and errors.
Improve TravisCI output by displaying files with warnings.
- Checking for nosetest xunit support now uses python-free directory …
…to avoid confusion.

- "Phase 6" heading now in one place, some info given about which test scheme is used.
- Check for nosetest commented.
Create separate sli_readline target
This library contains all the code necessary for GNUReadline interaction. We linke it only to sli and nest executable, so that for example pynest is not affected by readline linking stuff.
Comment sli_readline
and add implicit dependency to nestutil explicitly
Refactor CGConnect to take gidcollections
- Remove the variants of CGConnect that take subnets
- Refactor the variants of CGConnect to take gidcollections and (via conversions in SLI) also arrays and intvectors
- Remove convenience wrappers for internal function cgnext
- Adapt PyNEST wrapper for CGConnect
- Adapt and extend the test for CGConnect
Add SLI test for CGConnect
Please note that this test is not yet functional due to missing functionality in libcsa
@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Sep 5, 2017

Contributor

Just an additional note: this should probably not be merged before #817 was addressed. Currently, we are likely to miss proper testing of libneurosim/CSA related code.

Contributor

jougs commented Sep 5, 2017

Just an additional note: this should probably not be merged before #817 was addressed. Currently, we are likely to miss proper testing of libneurosim/CSA related code.

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Sep 7, 2017

Contributor

@heplesser: I have successfully reproduced your problem with libneurosim. Contrary to what I said before, I now think that this is a proper bug and not something that only occurs on some machines. I've opened #821 with some more explanations and a question for you.

Contributor

jougs commented Sep 7, 2017

@heplesser: I have successfully reproduced your problem with libneurosim. Contrary to what I said before, I now think that this is a proper bug and not something that only occurs on some machines. I've opened #821 with some more explanations and a question for you.

@heplesser heplesser added this to the NEST 2.14.0 milestone Oct 12, 2017

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Oct 13, 2017

Contributor

@heplesser I have addressed your remaining issues in the latest commits.

Add doxygen documentation for all functions in conngen.h

9601223 adds a Doxygen comment for cg_connect(). As all other functions were already documented in the .cpp file. I thus also added this one there. I hope this is OK.

conngenmodule.cpp:380: Do we really need to copy here? It seems the only thing we do with values is to loop over it and push its elements individually to the stack.

70d77c4 removes the copying.

conngenmodule.cpp:395: Where is the lock() that we unlock here---maybe comment?

The lock() is implicit as the object is derived from a lockptr. The doxygen comment there has an explanation, which I'd rather not repeat at the place of usage

conngen.cpp:92: Is it really safe to use &params[0] here and assume contiguous memory layout for data in a vector? I think the standard does not absolutely demand it; a C-style array would be safe.

This answer on Stack Overflow says that this guarantee was missing in C++98 but added later. I assume it is safe enough as I can only think of rather complicated ways a compiler should implement a vector non-contiguously given all its other properties.

conngen.cpp:104: Here we assume without checking that d_idx and w_idx are either 0 or 1. Is that safe?

156b88b adds a check and a corresponding exception for this.

Convert the manualtest to an example

a835bd4 converts the test to an example.

Contributor

jougs commented Oct 13, 2017

@heplesser I have addressed your remaining issues in the latest commits.

Add doxygen documentation for all functions in conngen.h

9601223 adds a Doxygen comment for cg_connect(). As all other functions were already documented in the .cpp file. I thus also added this one there. I hope this is OK.

conngenmodule.cpp:380: Do we really need to copy here? It seems the only thing we do with values is to loop over it and push its elements individually to the stack.

70d77c4 removes the copying.

conngenmodule.cpp:395: Where is the lock() that we unlock here---maybe comment?

The lock() is implicit as the object is derived from a lockptr. The doxygen comment there has an explanation, which I'd rather not repeat at the place of usage

conngen.cpp:92: Is it really safe to use &params[0] here and assume contiguous memory layout for data in a vector? I think the standard does not absolutely demand it; a C-style array would be safe.

This answer on Stack Overflow says that this guarantee was missing in C++98 but added later. I assume it is safe enough as I can only think of rather complicated ways a compiler should implement a vector non-contiguously given all its other properties.

conngen.cpp:104: Here we assume without checking that d_idx and w_idx are either 0 or 1. Is that safe?

156b88b adds a check and a corresponding exception for this.

Convert the manualtest to an example

a835bd4 converts the test to an example.

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Oct 13, 2017

Contributor

Please note that my latest commits are rather untested due to INCF/csa#6.

Contributor

jougs commented Oct 13, 2017

Please note that my latest commits are rather untested due to INCF/csa#6.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Oct 14, 2017

Contributor

Nice that you split this up, makes it a lot more comprehensible! The bools could even be const.

Contributor

heplesser commented on conngen/conngen.cpp in 156b88b Oct 14, 2017

Nice that you split this up, makes it a lot more comprehensible! The bools could even be const.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Oct 14, 2017

Contributor

@jougs Thanks for your fixes! Travis was unstable, I have restarted the builds there (failed again).

Concerning the doxygen comment placement, we can leave it where it is now, but generally my understanding is that class and method documentation should be in the header file, since the header might be all that developers see (a bit of closed source thinking here, but I think it makes sense).

Concerning the testing: How much work would it be to create a very simple connection generation code, so that one at least would have basic testing in place?

Contributor

heplesser commented Oct 14, 2017

@jougs Thanks for your fixes! Travis was unstable, I have restarted the builds there (failed again).

Concerning the doxygen comment placement, we can leave it where it is now, but generally my understanding is that class and method documentation should be in the header file, since the header might be all that developers see (a bit of closed source thinking here, but I think it makes sense).

Concerning the testing: How much work would it be to create a very simple connection generation code, so that one at least would have basic testing in place?

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Oct 15, 2017

Contributor

@jougs Thanks to advice from @lekshmideepu I now realised that the problem causing Travis to fail is that the autogen.sh process for CSA fails because README is missing in CSA (it has README.md). One way to fix this for now would be to create a symlink from README to README.md in our yaml file. Alternatively, should we temporarily replace CSA by some stub connectivity generator for test purposes?

See also CSA Issue 7.

Contributor

heplesser commented Oct 15, 2017

@jougs Thanks to advice from @lekshmideepu I now realised that the problem causing Travis to fail is that the autogen.sh process for CSA fails because README is missing in CSA (it has README.md). One way to fix this for now would be to create a symlink from README to README.md in our yaml file. Alternatively, should we temporarily replace CSA by some stub connectivity generator for test purposes?

See also CSA Issue 7.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Oct 15, 2017

Contributor

@jougs With CSA issue 7 fixed it looks pretty fine now, just some clang-formatting issues to fix.

Contributor

heplesser commented Oct 15, 2017

@jougs With CSA issue 7 fixed it looks pretty fine now, just some clang-formatting issues to fix.

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Oct 16, 2017

Contributor

Thanks for the review. I've addressed the remaining issues as follows:

  • 892c7fe adds const for the bools and fixes formatting
  • 1cf4325 fixes the testing issues by setting LD_LIBRARY_PATH in the build script
  • 5898e37 improves the output of the CSA test and removes the libcsa test. It can be re-added once libcsa is available in the wild
Contributor

jougs commented Oct 16, 2017

Thanks for the review. I've addressed the remaining issues as follows:

  • 892c7fe adds const for the bools and fixes formatting
  • 1cf4325 fixes the testing issues by setting LD_LIBRARY_PATH in the build script
  • 5898e37 improves the output of the CSA test and removes the libcsa test. It can be re-added once libcsa is available in the wild
@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Oct 16, 2017

Contributor

@jougs Great work! But I am confused after a look at the test configuration including libneurosim: There is says (l 5868ff):

  One-to-one connectivity using CGConnect with id intvectors ... SKIP: Python CSA package is not available
  One-to-one connectivity using CGConnect with paramters ... SKIP: Python CSA package is not available
  One-to-one connectivity using CGConnect with synmodel ... SKIP: Python CSA package is not available
  One-to-one connectivity using CGConnect with id tuples ... SKIP: Python CSA package is not available
  Error handling of CGConnect in case of unknown nodes ... SKIP: Python CSA package is not available
  Error handling of CGConnect in case of unknown synapse model ... SKIP: Python CSA package is not available

So the tests in test_csa.py are apparently not run on Travis, maybe due to a problem with Python paths? Those tests should run, shouldn't they? Could you check this?

Contributor

heplesser commented Oct 16, 2017

@jougs Great work! But I am confused after a look at the test configuration including libneurosim: There is says (l 5868ff):

  One-to-one connectivity using CGConnect with id intvectors ... SKIP: Python CSA package is not available
  One-to-one connectivity using CGConnect with paramters ... SKIP: Python CSA package is not available
  One-to-one connectivity using CGConnect with synmodel ... SKIP: Python CSA package is not available
  One-to-one connectivity using CGConnect with id tuples ... SKIP: Python CSA package is not available
  Error handling of CGConnect in case of unknown nodes ... SKIP: Python CSA package is not available
  Error handling of CGConnect in case of unknown synapse model ... SKIP: Python CSA package is not available

So the tests in test_csa.py are apparently not run on Travis, maybe due to a problem with Python paths? Those tests should run, shouldn't they? Could you check this?

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Oct 16, 2017

Contributor

D'oh!

The commands make and make install were missing from the CSA recipe. I've added them in 1e423f5. Let's see if this solves the issue with not running the test.

Contributor

jougs commented Oct 16, 2017

D'oh!

The commands make and make install were missing from the CSA recipe. I've added them in 1e423f5. Let's see if this solves the issue with not running the test.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Oct 16, 2017

Contributor

Well done! And merging ...

Contributor

heplesser commented Oct 16, 2017

Well done! And merging ...

@heplesser heplesser merged commit dfa5db2 into nest:master Oct 16, 2017

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Oct 16, 2017

Contributor

Many thanks to all who made this long story come to a good end. What a relief :-)

Contributor

jougs commented Oct 16, 2017

Many thanks to all who made this long story come to a good end. What a relief :-)

@jougs jougs deleted the jougs:refactor_conngen_interface branch Oct 16, 2017

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