Remove superfluous data structure for secondary events

[jougs]

This PR does the following:

• move the definitions/declarations for secondary events to their own file nestkernel/secondary_event.h
• remove the data structure for keeping the secondary events from the ModelManager in favor of the events that are stored in the connection models
• remove all pertaining functions for creating and deleting secondary events

On the way, I cleaned up the includes in the ModelsModule and some other minor things.

Update on October 18:

• clean up the handling of supported synapse IDs in SecondaryEvents
• remove the pristine_supported_syn_ids from SecondaryEvents
• simplify checks in ConnectionManager::connect_()
• protect a variable in DynamicLoaderModule from being used undefined

Fixes #2492.

[heplesser]

@jougs Thanks for tidying this up. I am still a bit in doubt why it is okay to remove the create secondary machinery as you do. Maybe easier to discuss that one-to-one.

[jougs]

@heplesser, @med-ayssar: Thanks for your valuable input.

In a video call with @heplesser and @suku248 on Friday, we concluded that these changes probably don't go far enough and we should revisit the handling of secondary events more thoroughly. I will mark this as draft for now with the option of either adding more commits later on or closing it in favor of another PR.

[jougs]

I have added a bunch of new commits to this. It should be much more complete now. The description was updated to explain what was added in detail. This should now also fix #2492.

[med-ayssar]

It seems that my changes here also fix the same issue #2492, and interestingly, we both have the same exact failing tests in Full/ OpenMP_only tests.

I will stop using my branch, and create a PR against this branch, if I will find something useful to fix the new failing tests.

[jougs]

@med-ayssar: I don't have any failures locally anymore, let's see what the CI thinks.

[heplesser]

@jougs Nice work! I added a few more comments. Unfortunately, I haven't spotted yet where things go wrong :(.

[jougs]

@heplesser: I've addressed your concerns in my latest commits and comments

[jougs]

@jougs Nice work! I added a few more comments. Unfortunately, I haven't spotted yet where things go wrong :(.

@heplesser: What do you mean by "things go wrong?" I don't see anything going wrong anymore.

[heplesser]

@jougs Nice work! I added a few more comments. Unfortunately, I haven't spotted yet where things go wrong :(.

@heplesser: What do you mean by "things go wrong?" I don't see anything going wrong anymore.

When I reviewed, the most recent run of the testsuite on Github still showed many failing tests, but clearly that is fixed now :).

[heplesser]

@jougs Could you remove the [:66] in test_copy_model.py so that that test also includes the secondary connections (and remove the comment)? In that way this test will check that #2492 is solved.

[jougs]

@jougs Could you remove the [:66] in test_copy_model.py so that that test also includes the secondary connections (and remove the comment)? In that way this test will check that #2492 is solved.

Removed in a63a3bb. However, I'm a bit shocked that this was even in (in master). If we have a bug, we ave a bug and I don't think that such a workaround should ever be in master just to allow other PRs to go in. How would I even have known that this needs to be removed if you did not tell me?

[heplesser]

Removed in a63a3bb. However, I'm a bit shocked that this was even in (in master). If we have a bug, we ave a bug and I don't think that such a workaround should ever be in master just to allow other PRs to go in. How would I even have known that this needs to be removed if you did not tell me?

I agree with you that this is borderline and we need to discuss how to handle situations like this in the future. In this case, #2379 fixed a concrete bug (problems copying all kinds of models), but also brought and improved test that detected the error described in #2492. This left the choice of either delaying #2379 until #2492 was fixed or weakening the test. I opted for the latter and put

#2492 (comment)

into #2492 so that we would not forget to remove the restriction from the test. That info would probably have been more visible in the issue description, maybe even in form of a to-do list.