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.

Sign up for GitHub

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

Jump to bottom

Ensure that ResetKernel resets off_grid_spiking_ #716

Merged
merged 3 commits into from
May 9, 2017
Merged

Ensure that ResetKernel resets off_grid_spiking_ #716

merged 3 commits into from
May 9, 2017

Conversation

suku248
Copy link
Contributor

@suku248 suku248 commented May 5, 2017

The default of the kernel parameter off_grid_spiking_ is false. The parameter can be altered by the user and it is automatically set to true if a precise neuron model is created.

So far, ResetKernel did not result in a reset of off_grid_spiking_ to false, which I find counterintuitive.

heplesser
heplesser reviewed May 8, 2017
View reviewed changes
nestkernel/event_delivery_manager.cpp Outdated
@@ -69,6 +69,8 @@ EventDeliveryManager::~EventDeliveryManager()
void
EventDeliveryManager::initialize()
{
off_grid_spiking_ =
false; // ensures that ResetKernel resets off_grid_spiking_
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be nicer to place the comment before the statement, so that the statement itself fits on a single line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@heplesser heplesser requested review from heplesser and jakobj May 8, 2017 12:42
@heplesser heplesser added ZC: Kernel DO NOT USE THIS LABEL I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) ZP: PR Created DO NOT USE THIS LABEL S: High Should be handled next T: Bug Wrong statements in the code or documentation labels May 8, 2017
@heplesser
Copy link
Contributor

@suku248 Thanks for detecting this! But somehow this makes test_spike_generator.sli fail. That may be due to a weakness in the test.

I also noticed that initialization and reset of EventDeliveryManager appears to be implemented inconsistently. From a quick check, it appears that

  • secondary_event_buffer_ is not initialized in the constructor
  • secondary_event_buffer_, spike_register_, offgrid_spike_register_, displacements_ and comm_marker_ are neither reset in finalize() or initialize()
  • All of those except comm_marker_ are cleared by cofigure_spike_buffers(), but that is called directly only from reset_network(), prepare_simulation() and set_status().

Could you check that all is well and add a comment why we do not need to reset those members explicitly (or fix the code if we do ...)?

@suku248 suku248 mentioned this pull request May 8, 2017
Closed
@suku248
Copy link
Contributor Author

suku248 commented May 8, 2017

The problem with the test was that two of the sub-tests required precise_times of the spike_detector to be true such that offsets could be retrieved from the spike_detector after the simulation. However, the two sub-tests did neither explicitly set precise_times to true nor globally enable off_grid_spiking. The test worked before as another (earlier) sub-test enables off_grid_spiking and ResetKernel at the beginning of the two sub-tests did not reset off_grid_spiking to false.

I agree that initialization and reset of the EventDeliveryManager need some code review but not as part of this PR. I have opened issue #717.

@heplesser
Copy link
Contributor

@suku248 Thanks for fixing the test and for opening #717!

@jakobj
Copy link
Contributor

jakobj commented May 9, 2017

Looks good! 👍 from me

@heplesser heplesser merged commit 7d3c92f into nest:master May 9, 2017
@suku248 suku248 deleted the reset_offgrid_spiking branch May 10, 2017 10:35
apdavison added a commit to apdavison/PyNN that referenced this pull request Nov 26, 2018
@apdavison
Trying to fix problem caused by nest/nest-simulator#716
7af1793
pgleeson added a commit to pgleeson/PyNN that referenced this pull request Mar 19, 2019
@pgleeson
Merge branch 'master' into serialization
1ef6f94 
* master:
  Add more information about random numbers (cf NeuralEnsemble#621)
  Update installation docs to reflect support for NEST 2.16.0
  FromListConnector now accepts empty lists, as it is supposed to.
  ...aaand forgot to add this too.
  Release 0.9.3
  Documentation update
  Forgot to add this to Git when doing the last release
  Fixed outdated documentation
  Don't use `close_fds=True`
  More robust test (changed connection ordering in NEST 2.16.0 should not break test)
  Workaround for what seems to be a regression in NEST 2.16.0
  Partially fixed a bug revealed by the stricter range checking of Neo 0.7.
  It appears that in NEST 2.16.0, "threads" must be set before "spike_precision"
  Trying to fix problem caused by nest/nest-simulator#716
  Updating NEST extension models to work with NEST 2.16
  Testing with NEST 2.16
  Added quorum requirement.
  Added document on project governance
  Added example of using IntFire1 model
  Added support for NEURON "ARTIFICIAL_CELL"-type neuron models.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakobj jakobj jakobj approved these changes

@heplesser heplesser heplesser approved these changes

Assignees
No one assigned
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: High Should be handled next T: Bug Wrong statements in the code or documentation ZC: Kernel DO NOT USE THIS LABEL ZP: PR Created DO NOT USE THIS LABEL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@suku248 @heplesser @jakobj