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

Adding inhomogeneous poisson generator model #671

Merged
merged 15 commits into from Mar 10, 2018

Conversation

@zbarni

zbarni commented Mar 4, 2017

Model for generating Poisson spikes with varying rate. Authorship shared with Renato Duarte (@rcfduarte).

@heplesser

@zbarni This looks quite fine, but I have some comments in the code, including one which is serious, I believe (in the update() method). You should also include a test, including corner cases.

Show outdated Hide outdated models/inh_poisson_generator.cpp
Show outdated Hide outdated models/inh_poisson_generator.cpp
Show outdated Hide outdated models/inh_poisson_generator.cpp
Show outdated Hide outdated models/inh_poisson_generator.cpp
Show outdated Hide outdated models/inh_poisson_generator.cpp
Show outdated Hide outdated models/inh_poisson_generator.cpp
Show outdated Hide outdated models/inh_poisson_generator.h
Show outdated Hide outdated models/inh_poisson_generator.h
Show outdated Hide outdated models/inh_poisson_generator.h
Show outdated Hide outdated models/inh_poisson_generator.h
@jschuecker

@zbarni, nice work! I added comments in the code and also commented on the critical issue raised by @heplesser.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Jun 1, 2017

Contributor

@zbarni This concerning the times at which rates change has now also popped up in #740. We must move all times to the grid, using the same mechanism as in spike_generator. precise_times are irrelevant here, since this generator is strictly interval-based (poisson_generator_ps is for poisson processes with precise times, but that is another story). It makes sense to handle allow_offgrid_times as in spike_generator. It is important to check that the spike times after conversion to Time objects on the grid are strictly increasing.

Contributor

heplesser commented Jun 1, 2017

@zbarni This concerning the times at which rates change has now also popped up in #740. We must move all times to the grid, using the same mechanism as in spike_generator. precise_times are irrelevant here, since this generator is strictly interval-based (poisson_generator_ps is for poisson processes with precise times, but that is another story). It makes sense to handle allow_offgrid_times as in spike_generator. It is important to check that the spike times after conversion to Time objects on the grid are strictly increasing.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Jun 1, 2017

Contributor

@zbarni Please run extras/check_code_style.sh on your code and fix all formatting issues!

Contributor

heplesser commented Jun 1, 2017

@zbarni Please run extras/check_code_style.sh on your code and fix all formatting issues!

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Oct 30, 2017

Contributor

@zbarni Please address the problems in this PR by 12 November or we will close this PR due to inactivity.

Contributor

heplesser commented Oct 30, 2017

@zbarni Please address the problems in this PR by 12 November or we will close this PR due to inactivity.

@zbarni

This comment has been minimized.

Show comment
Hide comment
@zbarni

zbarni Nov 8, 2017

@heplesser sorry for the extreme delays with this PR, but I have been working on my master's thesis in the last months and didn't really get a chance to look at the revisions. Would it be possible the keep the PR open for a few more weeks, maybe early December? I would implement the requested changes and some tests after my deadline, which is end of this month. It would be a pity not to have this PR go through as most of the work is already done.

zbarni commented Nov 8, 2017

@heplesser sorry for the extreme delays with this PR, but I have been working on my master's thesis in the last months and didn't really get a chance to look at the revisions. Would it be possible the keep the PR open for a few more weeks, maybe early December? I would implement the requested changes and some tests after my deadline, which is end of this month. It would be a pity not to have this PR go through as most of the work is already done.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Nov 9, 2017

Contributor

@zbarni Thank you for the explanation. We will keep the PR open for now, but at most until the end of the year.

Contributor

heplesser commented Nov 9, 2017

@zbarni Thank you for the explanation. We will keep the PR open for now, but at most until the end of the year.

@zbarni

This comment has been minimized.

Show comment
Hide comment
@zbarni

zbarni Dec 27, 2017

I'm not sure why the current commit(s) fails with cppcheck.. it complains about not finding all the include files. Can somebody take a quick look at the build? It might be related to #871.

zbarni commented Dec 27, 2017

I'm not sure why the current commit(s) fails with cppcheck.. it complains about not finding all the include files. Can somebody take a quick look at the build? It might be related to #871.

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Jan 12, 2018

Contributor

The problem is not caused by cppcheck, but rather by an error in the new test test_inhomogeneous_poisson_generator.sli:

Dec 24 14:01:51 dup [Error]: StackUnderflow

Probably the error can even be reproduced locally using make installcheck. Could you please check? Thanks!

Contributor

jougs commented Jan 12, 2018

The problem is not caused by cppcheck, but rather by an error in the new test test_inhomogeneous_poisson_generator.sli:

Dec 24 14:01:51 dup [Error]: StackUnderflow

Probably the error can even be reproduced locally using make installcheck. Could you please check? Thanks!

@zbarni

This comment has been minimized.

Show comment
Hide comment
@zbarni

zbarni Jan 15, 2018

Thanks for taking a look! Yes, the error is indeed there, but I've no idea why it happens. It's a very simple dummy test case, and when I run the same code (create a generator, set the parameters, and simulate for 10 ms) in a regular python script it runs without errors.

zbarni commented Jan 15, 2018

Thanks for taking a look! Yes, the error is indeed there, but I've no idea why it happens. It's a very simple dummy test case, and when I run the same code (create a generator, set the parameters, and simulate for 10 ms) in a regular python script it runs without errors.

@stinebuu

This comment has been minimized.

Show comment
Hide comment
@stinebuu

stinebuu Jan 30, 2018

Contributor

@zbarni I don't think the problem with the test is that there is an error per se, it is because with assert_or_die you have to have something to actually test, usually with the eq function, which checks for equality. A simple example can be found here, which asserts that when you apply round to 1.4 it equals 1. A bit more complex example is here in test_spike_detector.sli, which test that precision is what we expect it to be.

Your test runs, but then it comes to assert_or_die and gives the StackUnderflow because it is missing something to assert.

Hope this helps!

Contributor

stinebuu commented Jan 30, 2018

@zbarni I don't think the problem with the test is that there is an error per se, it is because with assert_or_die you have to have something to actually test, usually with the eq function, which checks for equality. A simple example can be found here, which asserts that when you apply round to 1.4 it equals 1. A bit more complex example is here in test_spike_detector.sli, which test that precision is what we expect it to be.

Your test runs, but then it comes to assert_or_die and gives the StackUnderflow because it is missing something to assert.

Hope this helps!

@zbarni

This comment has been minimized.

Show comment
Hide comment
@zbarni

zbarni Feb 5, 2018

@stinebuu thanks for the hint, it works now!
@heplesser I might have overlooked something, but I guess all the changes that were requested are now implemented. Please let me if anything has to be corrected / modified. Thank you!

EDIT: maybe one question: what should be the expected behavior when trying to set a rate at time 0.?
Currently this silently ignored as the rate will be set/stored but there will be no spikes generated..

zbarni commented Feb 5, 2018

@stinebuu thanks for the hint, it works now!
@heplesser I might have overlooked something, but I guess all the changes that were requested are now implemented. Please let me if anything has to be corrected / modified. Thank you!

EDIT: maybe one question: what should be the expected behavior when trying to set a rate at time 0.?
Currently this silently ignored as the rate will be set/stored but there will be no spikes generated..

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Feb 13, 2018

Contributor

@zbarni The code looks almost fine now, except for the problem with setting a rate at 0. Since that rate is silently ignored, this will lead to errors: Users will expect the generator to run with a given rate from time 0, and will likely not notice that the generator isn't emitting any spikes. That is dangerous and we need to prevent it.

The simplest approach would be to allow only rate times that are in the future. We could also allow setting the rate "now"; to support this, maybe calibrate() can check what rate should apply in the beginning.

Contributor

heplesser commented Feb 13, 2018

@zbarni The code looks almost fine now, except for the problem with setting a rate at 0. Since that rate is silently ignored, this will lead to errors: Users will expect the generator to run with a given rate from time 0, and will likely not notice that the generator isn't emitting any spikes. That is dangerous and we need to prevent it.

The simplest approach would be to allow only rate times that are in the future. We could also allow setting the rate "now"; to support this, maybe calibrate() can check what rate should apply in the beginning.

@zbarni

This comment has been minimized.

Show comment
Hide comment
@zbarni

zbarni Feb 26, 2018

@heplesser I added a check to make sure that rate times are strictly in the future, including a test case. This avoids the problem at 0., and I think it's an acceptable limitation for the users. However, I can also implement support for 'now' if you think it's necessary.

zbarni commented Feb 26, 2018

@heplesser I added a check to make sure that rate times are strictly in the future, including a test case. This avoids the problem at 0., and I think it's an acceptable limitation for the users. However, I can also implement support for 'now' if you think it's necessary.

@terhorstd terhorstd added this to the NEST 2.16 milestone Mar 5, 2018

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Mar 7, 2018

Contributor

@jschuecker Could you re-check this PR and approve it if all is in order? Then it could go into 2.16.

Contributor

heplesser commented Mar 7, 2018

@jschuecker Could you re-check this PR and approve it if all is in order? Then it could go into 2.16.

@heplesser heplesser merged commit 635a0d3 into nest:master Mar 10, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment