Fix STDP k-value error for edge case

[JanVogelsang]

PR #2443 contained a bug in the fix, this PR hotfixes the bugfix.
Adding max_delay_ + kernel().connection_manager.get_min_delay() looked correct, but was actually adding double + long, i.e., ms + delay.
It had to be max_delay_ + Time::delay_steps_to_ms( kernel().connection_manager.get_min_delay() ) instead.

[clinssen]

Can we amend the unit test so it fails on the old code, but passes on the new code?

[JanVogelsang]

I improved the test to correctly detect the original error correctly, tested some more parameter combinations and fixed some bugs in the test which occurred for that parameters. Additionally, I added a test for the stdp_pl_synapse_hom which is quite similar to the regular STDP test, mainly with different STDP weight dynamics.
It is not possible to make the test fail for the previous version of the bugfix, as it actually fixed the bug, but instead of archiving the spikes too short, it archived them longer than required (at least for resolutions of <1).

[JanVogelsang]

Strange, NEST aborts, but why? How can I debug this issue when it doesn't occur locally?

[heplesser]

Strange, NEST aborts, but why? How can I debug this issue when it doesn't occur locally?

Do you have the same setup locally (Python version etc) as on the CI runner? And have you tried to run the test locally as the test runner would do it, i.e., with python -m pytest --verbose --timeout 30 --numprocesses=1 test_stdp_synapse.py?

[github-actions]

Pull request automatically marked stale!

[clinssen]

@JanVogelsang : could you pull master and fix the merge conflicts? There is also one "TODO" marked in the code, could you re-enable plotting? Cheers!

[JanVogelsang]

Sure thing! I removed some warnings while I was at it, and don't get any errors or warning locally when I run pytest the same way the CI runner does (although with a higher Python version, I use 3.11).
However, enabling plotting slows down the test significantly. It takes multiple minutes to complete with plots and just a few second without plots. In CI it doesn't make sense to use plots anyway, so I would suggest we let the user enable them on-demand instead.

[clinssen]

Could it be that some unrelated changes were pulled in in connector_base.h and event_delivery_manager.cpp (and some other kernel files)?

[JanVogelsang]

You are right, those were actually incorrectly added.
And there are some other changes that are unrelated, but not worth their own PR. Just some lines that Pooja forgot in a recent PR (#2618).

[clinssen]

Thanks!

[JanVogelsang]

@abigailm Can you review this PR?

[JanVogelsang]

@abigailm Now that you are back from vacation, can you review this?

[abigailm]

All looks fine to me