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

Warn of SetStatus() between Prepare() and Cleanup() more thoroughly #1772

Closed
wants to merge 2 commits into from

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