Skip to content

Port issue-545.sli to py#2769

Merged
heplesser merged 3 commits intonest:masterfrom
otcathatsya:port_issue_545
Aug 30, 2023
Merged

Port issue-545.sli to py#2769
heplesser merged 3 commits intonest:masterfrom
otcathatsya:port_issue_545

Conversation

@otcathatsya
Copy link
Contributor

@otcathatsya otcathatsya commented May 5, 2023

It looks like the original SLI test may not have covered the intended error case to begin with? The original report specifies that passing integers leads to a crash/exception, though the SLI test seems to use floats only. In either case this is not an issue with python, as even integers are correctly cast.

Edit: it appears that a 2021 commit removed the integer part of the original SLI test since it became obsolete, purpose of the current test is a bit questionable

@otcathatsya otcathatsya changed the title Port SLI issue 545 to py Port issue-545.sli to py May 5, 2023
@heplesser
Copy link
Contributor

The key point here was that prior to #704 which fixed #545, NEST would actually crash when SetDefaults() contained unacceptable values. This would happen because we did not catch exceptions inside threads. A proper test for this regression would be to use parameters forcing an exception (e.g., setting a time constant to a negative value) and test for this with pytest.raises(). If NEST then crashed, the test would fail. We don't need to include passing cases here, and we only need to do this for a case with > 1 thread.

@nicolossus nicolossus requested a review from heplesser June 28, 2023 13:07
@nicolossus
Copy link
Member

@otcathatsya Can you please pull master into this branch so that you get the CI pipeline with the black checker enabled?

@nicolossus nicolossus added S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Aug 24, 2023
@heplesser heplesser merged commit bfc9841 into nest:master Aug 30, 2023
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: Maintenance Work to keep up the quality of the code and documentation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments