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

bug fix iaf_psc_exp_multisynapse / sli2py test_iaf_psc_exp_multisynapse #2986

Merged
merged 22 commits into from
Dec 12, 2023

Conversation

janskaar
Copy link
Contributor

Before, S_.current_ was computed before the updating individual synaptic ports, causing them to be 1 time step behind. Additionally, it was only computed when the neuron was not refractory, while the individual synaptic ports were updated every time step. Moving the S_.current_ computation down a few lines fixes this.

Port test_iaf_psc_exp_multisynapse.sli to py

@janskaar janskaar added T: Bug Wrong statements in the code or documentation S: Normal Handle this with default priority I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Nov 14, 2023
Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@janskaar Good catch! I have an alternative solution for summing the currents and some suggestions for the tests. The same problem applies to iaf_psc_alpha_multisynapse. I think it would be a good idea to fix both neurons within this PR.

testsuite/pytests/test_iaf_psc_exp_multisynapse.py Outdated Show resolved Hide resolved
testsuite/pytests/test_iaf_psc_exp_multisynapse.py Outdated Show resolved Hide resolved
testsuite/pytests/test_iaf_psc_exp_multisynapse.py Outdated Show resolved Hide resolved
testsuite/pytests/test_iaf_psc_exp_multisynapse.py Outdated Show resolved Hide resolved
testsuite/pytests/test_iaf_psc_exp_multisynapse.py Outdated Show resolved Hide resolved
testsuite/pytests/test_iaf_psc_exp_multisynapse.py Outdated Show resolved Hide resolved
testsuite/pytests/test_iaf_psc_exp_multisynapse.py Outdated Show resolved Hide resolved
models/iaf_psc_exp_multisynapse.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new test should got to pytest/sli2py_models.

janskaar and others added 11 commits November 22, 2023 13:23
Co-authored-by: Hans Ekkehard Plesser <hans.ekkehard.plesser@nmbu.no>
Co-authored-by: Hans Ekkehard Plesser <hans.ekkehard.plesser@nmbu.no>
Co-authored-by: Hans Ekkehard Plesser <hans.ekkehard.plesser@nmbu.no>
Co-authored-by: Hans Ekkehard Plesser <hans.ekkehard.plesser@nmbu.no>
Co-authored-by: Hans Ekkehard Plesser <hans.ekkehard.plesser@nmbu.no>
…multisynapse' into sli2py_iaf_psc_exp_multisynapse
Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@janskaar This looks fine now, but there seem to be some problems with formatting. Could you fix those? And how about a corresponding test for alpha multisynapse? Is that in a separate PR?

@janskaar
Copy link
Contributor Author

@janskaar This looks fine now, but there seem to be some problems with formatting. Could you fix those? And how about a corresponding test for alpha multisynapse? Is that in a separate PR?

Formatting should be fixed now. The alpha multisynapse test was merged a couple of weeks ago, but I have updated it to be more in line with this one now.

@janskaar
Copy link
Contributor Author

@heplesser Sorry, just noticed that test_mini_brunel_ps.sli fails, but only on MacOS. Have you encountered something like this before? It's for a completely different neuron model, so it's difficult to see why it should fail here.

@heplesser
Copy link
Contributor

@heplesser Sorry, just noticed that test_mini_brunel_ps.sli fails, but only on MacOS. Have you encountered something like this before? It's for a completely different neuron model, so it's difficult to see why it should fail here.

test_mini_brunel_ps.sli fails quite often. I just restarted it. The problem is in all likelihood that the data exchanged between the MPI ranks and the test runner, communicated via pipes, gets garbeled. A safer way would be to write the spike data to files and then base the test on the files, but I have kept delaying that change towards the Python version of the test. Maybe we can fix this next week.

@heplesser
Copy link
Contributor

@otcathatsya Ping!

Copy link
Contributor

@otcathatsya otcathatsya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Do we have a preference for which approximate comparison method to use? We have quite a few pytest.approx and I just saw that numpy's assert_almost_equal works with absolute decimal places instead, though in this case they'd work interchangeably too.

@heplesser
Copy link
Contributor

Looks good! Do we have a preference for which approximate comparison method to use? We have quite a few pytest.approx and I just saw that numpy's assert_almost_equal works with absolute decimal places instead, though in this case they'd work interchangeably too.

I don't think we have made a definite decision. I am not certain how pytest.approx works on NumPy arrays, because == between arrays returns an array of bools, not a single truth value.

@heplesser heplesser merged commit c4b3836 into nest:master Dec 12, 2023
24 checks passed
@janskaar janskaar deleted the sli2py_iaf_psc_exp_multisynapse branch December 21, 2023 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Bug Wrong statements in the code or documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants