Skip to content

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.

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
@janskaar janskaar requested a review from heplesser November 22, 2023 14:38
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 janskaar requested a review from heplesser November 25, 2023 13:47
@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
@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.

3 participants