Skip to content

Conversation

@jd41
Copy link
Contributor

@jd41 jd41 commented Sep 21, 2020

For me, the warning in the notes in that section was not enough. I changed some things in the simulation code I am working with, including replacing nest.Simulate() by nest.Prepare()/nest.Run()/nest.Cleanup(). After that, there was some SetStatus() between Prepare() and Cleanup(). I didn't catch the note that SetStatus() should not be called between Prepare() and Cleanup(). I continued making other changes, because the code seemed to continue to work. At some point, I realized that there were no/not enough spikes recorded anymore. Tracking down that the problem was in replacing Simulate() by Run() took me on the order of a week, and I only recognized the current warning in the documentation after reverting that change just to see what happens.

For me, the warning in the notes in that section was not enough. I changed some things in the simulation code I am working with, including replacing nest.Simulate() by nest.Prepare()/nest.Run()/nest.Cleanup(). After that, there was some SetStatus() between Prepare() and Cleanup(). I didn't catch the note that SetStatus() should not be called between Prepare() and Cleanup(). I continued making other changes, because the code seemed to continue to work. At some point, I realized that there were no/not enough spikes recorded anymore. Tracking down that the problem was in replacing Simulate() by Run() took me on the order of a week, and I only recognized the current warning in the documentation after reverting that change just to see what happens.
@Silmathoron
Copy link
Member

@apeyser would it be possible to detect this and raise an error? Did you already introduce variables that would tell that cleanup has not been called yet?
I think calling SetStatus in that case should directly raise an error, we should not let users be able to do this unknowingly...

@jd41
Copy link
Contributor Author

jd41 commented Sep 21, 2020

I wrote issue #1773, in which I propose/ask if it would work to do the prepare/run/cleanup stuff on the PyNEST level and hide that complexity from the PyNEST user.

@stinebuu stinebuu added 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. labels Sep 30, 2020
@stinebuu
Copy link
Contributor

stinebuu commented Nov 9, 2020

We need to fix the underlying issue (throwing error when SetStatus is called), thus we are not merging this. See also #1773.

@stinebuu stinebuu closed this Nov 9, 2020
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

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants