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

Add iaf_psc_exp_ps_lossless model by Krishnan et al #901

Merged
merged 51 commits into from Apr 13, 2018

Conversation

@heplesser
Contributor

heplesser commented Mar 7, 2018

This PR is a follow-up to the retired #648. Two issues remain at present:

  1. The test for the model fails, because the reference data are taken from the non-precise iaf_psc_exp model. We need reference data generated outside of NEST for this model.
  2. The test for the model is very simple, just response to DC input; it does not test any of the advanced features, in particular the losslessness.
  3. There is some problem with the copyright header, probably a technical problem with the test.
@ChristianKeup

This comment has been minimized.

Show comment
Hide comment
@ChristianKeup

ChristianKeup Mar 9, 2018

Contributor

Hi, so I was assigned by Moritz to help completing this pull request.
Since this is my first adventure into the nest code and I don't know SLI, pls be patient with me!
I would like to know what exactly you want me to do:

  • fix the unittest
  • expand the unittest to incorporate more features -> which ones?
  • try to fix the problem related to #368 just mentioned ?
  • more?

Also, do you want me to work directly with (i.e. push to) the heplesser:krishnanj-time-reversal-precise branch, or should I use my own branch that will then in the end be merged into nest:master? (sorry if this is clear)

Contributor

ChristianKeup commented Mar 9, 2018

Hi, so I was assigned by Moritz to help completing this pull request.
Since this is my first adventure into the nest code and I don't know SLI, pls be patient with me!
I would like to know what exactly you want me to do:

  • fix the unittest
  • expand the unittest to incorporate more features -> which ones?
  • try to fix the problem related to #368 just mentioned ?
  • more?

Also, do you want me to work directly with (i.e. push to) the heplesser:krishnanj-time-reversal-precise branch, or should I use my own branch that will then in the end be merged into nest:master? (sorry if this is clear)

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Mar 10, 2018

Contributor

@ChristianKeup Welcome aboard! You summarized the remaining tasks quite well. Some comments:

  • Currently, three tests are failing. We will work on the problem with ticket-659-copyright, that seems to be some strange technicality.
  • The issue-77.sli test is failing due to the problem related to #368; I will also try looking into it, but it would be good if you could take a look as well. As I wrote before, the problem is caused by spikes arriving at the same time with opposite signs.
  • test_iaf_psc_exp_ps_lossless.sli fails because the reference values stored in the file are not correct. Only those values need to be updated and replaced by correct values that have been obtained differently. Since in that test the model is only driven by constant input, it should be possible to obtain the correct solution analytically.
  • What is missing are tests that test whether the "new stuff" in the model works, i.e., the guaranteed detection of spikes also in corner cases. Does the paper contain examples of such cases we could use for the test? If you prefer, you could implement the extended tests in Python.

Concerning the practical procedure: This PR is from heplesser:krishnanj-time-reversal-precise, i.e., branch krishnanj-time-reversal-precise in my Github fork of NEST (https://github.com/heplesser/nest-simulator). You should check out that branch, commit your changes to your local copy of the branch, the push it to your NEST fork on Github and create a PR against my branch (not master). I can then integrate your changes and they will end up here in the PR against master.

Contributor

heplesser commented Mar 10, 2018

@ChristianKeup Welcome aboard! You summarized the remaining tasks quite well. Some comments:

  • Currently, three tests are failing. We will work on the problem with ticket-659-copyright, that seems to be some strange technicality.
  • The issue-77.sli test is failing due to the problem related to #368; I will also try looking into it, but it would be good if you could take a look as well. As I wrote before, the problem is caused by spikes arriving at the same time with opposite signs.
  • test_iaf_psc_exp_ps_lossless.sli fails because the reference values stored in the file are not correct. Only those values need to be updated and replaced by correct values that have been obtained differently. Since in that test the model is only driven by constant input, it should be possible to obtain the correct solution analytically.
  • What is missing are tests that test whether the "new stuff" in the model works, i.e., the guaranteed detection of spikes also in corner cases. Does the paper contain examples of such cases we could use for the test? If you prefer, you could implement the extended tests in Python.

Concerning the practical procedure: This PR is from heplesser:krishnanj-time-reversal-precise, i.e., branch krishnanj-time-reversal-precise in my Github fork of NEST (https://github.com/heplesser/nest-simulator). You should check out that branch, commit your changes to your local copy of the branch, the push it to your NEST fork on Github and create a PR against my branch (not master). I can then integrate your changes and they will end up here in the PR against master.

@stinebuu

This comment has been minimized.

Show comment
Hide comment
@stinebuu

stinebuu Mar 12, 2018

Contributor

@heplesser concerning ticket-659-copyright, there seems to be a whitespace after * test_iaf_psc_exp_ps_lossless.sli in the test file. Removing the whitespace makes the copyright error disappear.

Contributor

stinebuu commented Mar 12, 2018

@heplesser concerning ticket-659-copyright, there seems to be a whitespace after * test_iaf_psc_exp_ps_lossless.sli in the test file. Removing the whitespace makes the copyright error disappear.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Mar 14, 2018

Contributor

@stinebuu Thanks for the detective work!

Contributor

heplesser commented Mar 14, 2018

@stinebuu Thanks for the detective work!

@ChristianKeup

This comment has been minimized.

Show comment
Hide comment
@ChristianKeup

ChristianKeup Mar 19, 2018

Contributor

I have found a problem where the lossless neuron is behaving for small inputs close to threshold like a normal unprecise ias_psc_exp neuron. I'm currently still looking into it.

Contributor

ChristianKeup commented Mar 19, 2018

I have found a problem where the lossless neuron is behaving for small inputs close to threshold like a normal unprecise ias_psc_exp neuron. I'm currently still looking into it.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Mar 20, 2018

Contributor

@ChristianKeup I have just pushed a change that solves the problem with issue-77.sil by explicitly skipping state propagation and spike check if two spikes arrive at the same time, so that ministep==0. I don't know if that will help the issue you are working on. The problem with the copyright check is also fixed.

Contributor

heplesser commented Mar 20, 2018

@ChristianKeup I have just pushed a change that solves the problem with issue-77.sil by explicitly skipping state propagation and spike check if two spikes arrive at the same time, so that ministep==0. I don't know if that will help the issue you are working on. The problem with the copyright check is also fixed.

@ChristianKeup

This comment has been minimized.

Show comment
Hide comment
@ChristianKeup

ChristianKeup Mar 30, 2018

Contributor

The problem I mentioned turned out to be due to external currents, as from a /dc_generator, not being taken into account in the spike detection method is_spike( dt ). It only took into account the external current given by the model parameter P_.I_e_. (Is it on purpose that the two are treated seperately?)

In the process I checked the equations implemented in is_spike( dt ). They are in fact correct and working. However, it turned out that eq. (49) in Krishnan et al. 2018 is not correct. Also, in the spike detection algorithms 1 and 2 in the paper, the functions g and f are interchanged. (I have mailed with Luca about this)
I changed to code to be less misleading and added a short comment in case someone else should ever try to understand the implementation.

Now the model is working nicely. I fixed the current unittest and also added the exact same unittest for the lossy /iaf_psc_exp_ps since none yet existed. In this simple test, both models behave exactly the same, as they should. I will write an extension testing for the region of previously missed spikes next week.
@heplesser I've created a PR with the changes against your branch, hopefully in the correct way.

Contributor

ChristianKeup commented Mar 30, 2018

The problem I mentioned turned out to be due to external currents, as from a /dc_generator, not being taken into account in the spike detection method is_spike( dt ). It only took into account the external current given by the model parameter P_.I_e_. (Is it on purpose that the two are treated seperately?)

In the process I checked the equations implemented in is_spike( dt ). They are in fact correct and working. However, it turned out that eq. (49) in Krishnan et al. 2018 is not correct. Also, in the spike detection algorithms 1 and 2 in the paper, the functions g and f are interchanged. (I have mailed with Luca about this)
I changed to code to be less misleading and added a short comment in case someone else should ever try to understand the implementation.

Now the model is working nicely. I fixed the current unittest and also added the exact same unittest for the lossy /iaf_psc_exp_ps since none yet existed. In this simple test, both models behave exactly the same, as they should. I will write an extension testing for the region of previously missed spikes next week.
@heplesser I've created a PR with the changes against your branch, hopefully in the correct way.

ChristianKeup and others added some commits Apr 6, 2018

Merge pull request #22 from ChristianKeup/lossless
Changes to implementation and test of iaf_psc_exp_ps_lossless for Nest PR #901
@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Apr 7, 2018

Contributor

@Silmathoron @stinebuu I believe this PR is now in order. Could you have another look? For background on some of the changes and design decisions, please see also the discussion on heplesser#22

Contributor

heplesser commented Apr 7, 2018

@Silmathoron @stinebuu I believe this PR is now in order. Could you have another look? For background on some of the changes and design decisions, please see also the discussion on heplesser#22

@stinebuu

I found a tiny typo, otherwise it looks good.

Show outdated Hide outdated precise/iaf_psc_exp_ps_lossless.cpp
Update iaf_psc_exp_ps_lossless.cpp
Fixed typo in comment.
@Silmathoron

All good for me

@heplesser heplesser merged commit e24b13f into nest:master Apr 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@heplesser heplesser deleted the heplesser:krishnanj-time-reversal-precise branch Apr 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment