Refactoring NEST #205

Merged
merged 287 commits into from Jan 29, 2016

Conversation

Projects
None yet
10 participants
@tammoippen
Contributor

tammoippen commented Jan 12, 2016

This PR contains our joint effort on refactoring the kernel of NEST. Beside one public API change, the appearance and behavior of NEST should remain unchanged. The refactoring contains:

  • The classes Network, Scheduler, Communicator and ConnectionManager are marked deprecated and will be removed in the future.
  • Their functionality is moved into Manager classes:
    • ConnectionBuilderManager (will be renamed to ConnectionManager later) is responsible for storing, creating and managing connections.
    • EventDeliveryManager is responsible for delivering events, both local and remote.
    • IOManager is responsible for file IO with the operating system.
    • LoggingManager is responsible for creating and distributing logging events (former Network::message(...) method).
    • ModelManager is responsible for storing node and synapse models and creating new instances from these models.
    • ModelRangeManager stores the node model for ranges of nodes.
    • MPIManager is responsible for all MPI communication.
    • MUSICManager is responsible for all MUSIC related interactions.
    • NodeManager is responsible for storing and managing all node objects.
    • RNGManager is responsible for creation and access to all random number generators.
    • SimulationManager is responsible for starting, stopping and resuming simulations.
    • SPManager is responsible for structural plasticity.
    • VPManager is responsible for threading and the management of virtual processes.
  • The KernelManager gives easy access to all Manager classes via the function nest::kernel().
  • [API change] The min_delay and max_delay setting and checking is moved from the ConnectorModel to the DelayCheckers, which are managed by the ConnectionBuilderManager. Therefore, the *_delay is not set on the default synapse types any more, but during the SetKernelStatus.
  • Sort the header includes according to the guidelines. Also add the script extras/include_checker.py that suggests an include order for a file or the files in a directory.
  • Each module contain the beginning for a public NEST API, that can be used to program NEST on the C++ level. The corresponding SLIModules are implemented on top of this API (ongoing work).

I hope I did not miss any changes. I invite you all (@nest/nest-developers) to review and test this PR. @janhahne please see that the gap junctions work as expected. @sdiazpier and @naveau please see that the structural plasticity is working as expected.

tammoippen and others added some commits Sep 30, 2015

hannah
Merge branch 'refactoring_nest' of github.com:tammoippen/nest-private…
… into ConnectionBuilderManager

Conflicts:
	nestkernel/network.cpp
Maximilian Schmidt
added NodeManager class to nestkernel, moved functions and member var…
…iables from Network class to NodeManager, adapted function calls in all scripts
kunkel
Merge branch 'refactoring_nest' of github.com:tammoippen/nest-private…
… into modelmanager

Conflicts:
	nestkernel/Makefile.am
	nestkernel/connection_manager.cpp
	nestkernel/network.cpp
	nestkernel/network.h
Merge pull request #2 from mdjurfeldt/refactoring_nest
Refer to vp_manager.get_num_threads()
Maximilian Schmidt
fixed two bugs in node_manager: proper initialization of siblingconta…
…iner and return reference to nodes_vec_ in get_nodes_vec_
- Created Simulation Manager class
- Added virtual destructor to ManagerInterface base class
- Some code beautification.
@gewaltig

This comment has been minimized.

Show comment
Hide comment
@gewaltig

gewaltig Jan 12, 2016

Dear Tamo,

This looks like an impressive piece of work! Congratulations already now for taking on this Herculean task of cleaning up the classes.
I’ll definitely have a look at the PR.

Best
Marc-Oliver

Dear Tamo,

This looks like an impressive piece of work! Congratulations already now for taking on this Herculean task of cleaning up the classes.
I’ll definitely have a look at the PR.

Best
Marc-Oliver

@apeyser

This comment has been minimized.

Show comment
Hide comment
@apeyser

apeyser Jan 12, 2016

Contributor

Would this be worth rebasing onto the master?

Contributor

apeyser commented Jan 12, 2016

Would this be worth rebasing onto the master?

@janhahne

This comment has been minimized.

Show comment
Hide comment
@janhahne

janhahne Jan 13, 2016

Contributor

@tammoippen The tests from the testsuite are sufficient to confirm that the gap junctions work fine. From looking at the source code I can confirm that you missed nothing and that all of the later applied changes to the gap framework are present. Very good work!

Contributor

janhahne commented Jan 13, 2016

@tammoippen The tests from the testsuite are sufficient to confirm that the gap junctions work fine. From looking at the source code I can confirm that you missed nothing and that all of the later applied changes to the gap framework are present. Very good work!

@janhahne

This comment has been minimized.

Show comment
Hide comment
@janhahne

janhahne Jan 15, 2016

Contributor

@tammoippen I just noticed a small influence of this PR on the gap junction framework. The delay of gap_junction connections is no longer ignored despite of how they are created ( see issue #208 ). In 2.10.0 it worked at least for one case.

Contributor

janhahne commented Jan 15, 2016

@tammoippen I just noticed a small influence of this PR on the gap junction framework. The delay of gap_junction connections is no longer ignored despite of how they are created ( see issue #208 ). In 2.10.0 it worked at least for one case.

tammoippen and others added some commits Jan 18, 2016

Only check delay for `min/max_delay`, if model has delay.
This relates to #208, as gap junctions should not have an influence on the `min/max_delay`
Merge pull request #2 from janhahne/nest3
Updated gap_junction test
@sdiazpier

This comment has been minimized.

Show comment
Hide comment
@sdiazpier

sdiazpier Jan 19, 2016

Contributor

Dear @tammoippen I have tested all the structural plasticity code and it is working well. I have also checked all the related classes and everything seems to be in place for the structural plasticity functionality.

Contributor

sdiazpier commented Jan 19, 2016

Dear @tammoippen I have tested all the structural plasticity code and it is working well. I have also checked all the related classes and everything seems to be in place for the structural plasticity functionality.

nestkernel/Makefile.am
@@ -54,25 +52,23 @@ libnest_la_SOURCES=\
gid_collection.h gid_collection.cpp\
histentry.h histentry.cpp\
model.h model.cpp\
- nest.h\
+ model_manager.h model_manager.cpp\

This comment has been minimized.

@jakobj

jakobj Jan 21, 2016

Contributor

model_manager_impl.h is missing?

@jakobj

jakobj Jan 21, 2016

Contributor

model_manager_impl.h is missing?

nestkernel/Makefile.am
+ io_manager.h io_manager.cpp \
+ mpi_manager.h mpi_manager_impl.h mpi_manager.cpp \
+ simulation_manager.h simulation_manager.cpp \
+ connection_builder_manager.h connection_builder_manager.cpp \

This comment has been minimized.

@jakobj

jakobj Jan 21, 2016

Contributor

connection_builder_manager_impl.h is missing?

@jakobj

jakobj Jan 21, 2016

Contributor

connection_builder_manager_impl.h is missing?

@tammoippen

This comment has been minimized.

Show comment
Hide comment
@tammoippen

tammoippen Jan 25, 2016

Contributor

Hi @nest/nest-developers,

we plan to merge the PR by the end of Jan. Please have a look into the changes again and find errors or improvements. I added some graphics to help you fiddle your way through the managers:

ManagerRelations.pdf

ManagerDetails.pdf

Thank you! 😄

Contributor

tammoippen commented Jan 25, 2016

Hi @nest/nest-developers,

we plan to merge the PR by the end of Jan. Please have a look into the changes again and find errors or improvements. I added some graphics to help you fiddle your way through the managers:

ManagerRelations.pdf

ManagerDetails.pdf

Thank you! 😄

@jakobj

This comment has been minimized.

Show comment
Hide comment
@jakobj

jakobj Jan 27, 2016

Contributor

i think you might be missing the include guards in model_manager_impl.h and universal_data_logger.h

Contributor

jakobj commented Jan 27, 2016

i think you might be missing the include guards in model_manager_impl.h and universal_data_logger.h

@tammoippen

This comment has been minimized.

Show comment
Hide comment
@tammoippen

tammoippen Jan 27, 2016

Contributor

@jakobj Thanks for catching this. Actually, there were a few other *_impl.h missing include guards.

Contributor

tammoippen commented Jan 27, 2016

@jakobj Thanks for catching this. Actually, there were a few other *_impl.h missing include guards.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Jan 27, 2016

Contributor

I just compiled NEST3 with clang-omp from Homebrew under OSX 10.10. It spat out a number of warnings, and some tests fail.

@tammoippen How should I best contribute small fixes, such as missing newlines at end of file?

@sdiazpier Clang points out (correctly) that SPBuilder::connect(GIDCollection, GIDCollection) shadows ConnBuilder::connect(). Both appear to be rather different functions, since one takes two arguments and the other none. The two-argument version is apparently only called from SPManager::create_synapses(). So I think the function in SPBuilder should probably get a different name. A positive side effect would be that it is no longer virtual, making it (likely) faster.

@sdiazpier Clang also issues the following warning:

../../src/nestkernel/sp_manager.cpp:622:21: warning: comparison of integers of different signs: 'int' and 'size_type' (aka 'unsigned long') [-Wsign-compare]
    if ( -( *n_it ) > global_sources.size() )

@jougs Clang warns about a few deprecated MPI-2 names:

../../src/nestkernel/mpi_manager.cpp:116:3: warning: 'MPI_Address' is deprecated: MPI_Address is superseded by MPI_Get_address in MPI-2.0 [-Wdeprecated-declarations]
  MPI_Address( &( ogs.offset_ ), &address );
  ^
/usr/local/include/mpi.h:1187:20: note: 'MPI_Address' has been explicitly marked deprecated here
OMPI_DECLSPEC  int MPI_Address(void *location, MPI_Aint *address)
                   ^
../../src/nestkernel/mpi_manager.cpp:122:3: warning: 'MPI_Type_struct' is deprecated: MPI_Type_struct is superseded by MPI_Type_create_struct in MPI-2.0
      [-Wdeprecated-declarations]
  MPI_Type_struct( 2, blockcounts, offsets, source_types, &MPI_OFFGRID_SPIKE );

Unfortunately, several tests fail:

  • Running test 'unittests/test_aeif_cond_alpha_multisynapse.sli'... Failed: segmentation fault
  • Running test 'unittests/test_mip_corrdet.sli'... Failed: missed C++ assertion
  • Running test 'unittests/test_multithreading_devices.sli'... Failed: segmentation fault
  • Running test 'unittests/test_recorder_close_flush.sli'... Failed: missed SLI assertion

Finally the testsuite hangs forever on Running test 'regressiontests/ticket-80-175-179.sli'...
No tests later in the testsuite are run.

I haven't had time to investigate.

Contributor

heplesser commented Jan 27, 2016

I just compiled NEST3 with clang-omp from Homebrew under OSX 10.10. It spat out a number of warnings, and some tests fail.

@tammoippen How should I best contribute small fixes, such as missing newlines at end of file?

@sdiazpier Clang points out (correctly) that SPBuilder::connect(GIDCollection, GIDCollection) shadows ConnBuilder::connect(). Both appear to be rather different functions, since one takes two arguments and the other none. The two-argument version is apparently only called from SPManager::create_synapses(). So I think the function in SPBuilder should probably get a different name. A positive side effect would be that it is no longer virtual, making it (likely) faster.

@sdiazpier Clang also issues the following warning:

../../src/nestkernel/sp_manager.cpp:622:21: warning: comparison of integers of different signs: 'int' and 'size_type' (aka 'unsigned long') [-Wsign-compare]
    if ( -( *n_it ) > global_sources.size() )

@jougs Clang warns about a few deprecated MPI-2 names:

../../src/nestkernel/mpi_manager.cpp:116:3: warning: 'MPI_Address' is deprecated: MPI_Address is superseded by MPI_Get_address in MPI-2.0 [-Wdeprecated-declarations]
  MPI_Address( &( ogs.offset_ ), &address );
  ^
/usr/local/include/mpi.h:1187:20: note: 'MPI_Address' has been explicitly marked deprecated here
OMPI_DECLSPEC  int MPI_Address(void *location, MPI_Aint *address)
                   ^
../../src/nestkernel/mpi_manager.cpp:122:3: warning: 'MPI_Type_struct' is deprecated: MPI_Type_struct is superseded by MPI_Type_create_struct in MPI-2.0
      [-Wdeprecated-declarations]
  MPI_Type_struct( 2, blockcounts, offsets, source_types, &MPI_OFFGRID_SPIKE );

Unfortunately, several tests fail:

  • Running test 'unittests/test_aeif_cond_alpha_multisynapse.sli'... Failed: segmentation fault
  • Running test 'unittests/test_mip_corrdet.sli'... Failed: missed C++ assertion
  • Running test 'unittests/test_multithreading_devices.sli'... Failed: segmentation fault
  • Running test 'unittests/test_recorder_close_flush.sli'... Failed: missed SLI assertion

Finally the testsuite hangs forever on Running test 'regressiontests/ticket-80-175-179.sli'...
No tests later in the testsuite are run.

I haven't had time to investigate.

@tammoippen

This comment has been minimized.

Show comment
Hide comment
@tammoippen

tammoippen Jan 28, 2016

Contributor

How should I best contribute small fixes, such as missing newlines at end of file?

Please create a PR onto my Fork. I will merge ASAP.

Unfortunately, several tests fail:

Running test 'unittests/test_aeif_cond_alpha_multisynapse.sli'... Failed: segmentation fault
Running test 'unittests/test_mip_corrdet.sli'... Failed: missed C++ assertion
Running test 'unittests/test_multithreading_devices.sli'... Failed: segmentation fault
Running test 'unittests/test_recorder_close_flush.sli'... Failed: missed SLI assertion
Finally the testsuite hangs forever on Running test 'regressiontests/ticket-80-175-179.sli'...
No tests later in the testsuite are run.

Some of these tests also fail for NEST 2.10.0 on OS X with clang. I think there is a specific issue with this combination (on Linux with clang I did not observe any of these issues).

Contributor

tammoippen commented Jan 28, 2016

How should I best contribute small fixes, such as missing newlines at end of file?

Please create a PR onto my Fork. I will merge ASAP.

Unfortunately, several tests fail:

Running test 'unittests/test_aeif_cond_alpha_multisynapse.sli'... Failed: segmentation fault
Running test 'unittests/test_mip_corrdet.sli'... Failed: missed C++ assertion
Running test 'unittests/test_multithreading_devices.sli'... Failed: segmentation fault
Running test 'unittests/test_recorder_close_flush.sli'... Failed: missed SLI assertion
Finally the testsuite hangs forever on Running test 'regressiontests/ticket-80-175-179.sli'...
No tests later in the testsuite are run.

Some of these tests also fail for NEST 2.10.0 on OS X with clang. I think there is a specific issue with this combination (on Linux with clang I did not observe any of these issues).

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Jan 28, 2016

Contributor

I have explored the test failures a bit further (all with clang-omp):

  • test_aeif_cond_alpha_multisynapse.sli
    • fails on most runs with a segfault, usually somewhere during update()
    • detailed locations vary
    • on rare occasions, it does not crash, but appears to hang
    • in one case, it seg faulted during finalize_nodes()
    • the test uses only a single thread
  • test_mip_corrdet_sli
    • uses single thread
    • seg faults reliably
    • mostly in finalize_nodes(), once in update()
  • test_multithreading_devices.sli
    • uses 1 or 2 threads
    • seg faults reliably
    • test_multithreading.sli passes reliably
  • test_recorder_close_flush.sli
    • seg faults reliably
    • on one occasion, it ran for a while before segfaulting
  • ticket-80-175-179.sli
    • seg faults in most cases
    • hangs occasionally

Tests coming after ticket-80-175-179.sli have not run yet.

Contributor

heplesser commented Jan 28, 2016

I have explored the test failures a bit further (all with clang-omp):

  • test_aeif_cond_alpha_multisynapse.sli
    • fails on most runs with a segfault, usually somewhere during update()
    • detailed locations vary
    • on rare occasions, it does not crash, but appears to hang
    • in one case, it seg faulted during finalize_nodes()
    • the test uses only a single thread
  • test_mip_corrdet_sli
    • uses single thread
    • seg faults reliably
    • mostly in finalize_nodes(), once in update()
  • test_multithreading_devices.sli
    • uses 1 or 2 threads
    • seg faults reliably
    • test_multithreading.sli passes reliably
  • test_recorder_close_flush.sli
    • seg faults reliably
    • on one occasion, it ran for a while before segfaulting
  • ticket-80-175-179.sli
    • seg faults in most cases
    • hangs occasionally

Tests coming after ticket-80-175-179.sli have not run yet.

@jougs jougs referenced this pull request in tammoippen/nest-simulator Jan 28, 2016

Merged

Cleanup #5

@tammoippen

This comment has been minimized.

Show comment
Hide comment
@tammoippen

tammoippen Jan 29, 2016

Contributor

Thank you for finding and resolving all these issues. I addressed the remaining issues in #74 and #212 - I do not think, that they should block this PR. Hence, I am merging this PR and lifting the code freeze.

Contributor

tammoippen commented Jan 29, 2016

Thank you for finding and resolving all these issues. I addressed the remaining issues in #74 and #212 - I do not think, that they should block this PR. Hence, I am merging this PR and lifting the code freeze.

tammoippen added a commit that referenced this pull request Jan 29, 2016

@tammoippen tammoippen merged commit ff2bdbb into nest:master Jan 29, 2016

1 check passed

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

@heplesser heplesser referenced this pull request Feb 16, 2016

Closed

Calcium neuron model #176

@tammoippen tammoippen deleted the tammoippen:nest3 branch Feb 17, 2016

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