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

Fix ac_generator when used with start/stop (addresses #720) #730

Merged
merged 1 commit into from May 26, 2017

Conversation

Projects
None yet
3 participants
@heplesser
Contributor

heplesser commented May 22, 2017

Corrected ac_generator when used with start/stop, improved documentation and added test.

This addresses #720, although with a slightly different logic than apparently assumed by PyNN: In NEST, start and stop strictly do windowing and origin strictly moves this window. Thus, time is always counted from 0 when computing the sinusoid. This was incorrectly implemented until now and is now fixed.

Furthermore, for the parameters given in #720, the ac_generator now provides I=1000 pA at t=5 ms as expected.

@appukuttan-shailesh Would you be willing to review this PR? Please use the "Start a review" feature and the "Review changes" button to submit your comments and verdict. I cannot add you to the "Reviewers" since you currently are not a member of NEST on Github.

@appukuttan-shailesh

This comment has been minimized.

Show comment
Hide comment
@appukuttan-shailesh

appukuttan-shailesh May 23, 2017

Contributor

@heplesser : Sure, I will review this. About the membership, I saw that I'm listed here: https://github.com/nest/nest-simulator/network/members. Do I need to do anything else?

Contributor

appukuttan-shailesh commented May 23, 2017

@heplesser : Sure, I will review this. About the membership, I saw that I'm listed here: https://github.com/nest/nest-simulator/network/members. Do I need to do anything else?

@appukuttan-shailesh

@heplesser : I was running this test code, and I find that the signal is phase shifted by 180:
ac_180phase
If I compensate the phase shift now by specifying: 'phase' : 180.0, then it is as expected, with the currents being correctly evaluated as:

t (ms) <-> I (pA)
4.9    <-> 0.0
5.0    <-> 1000.00
5.1    <-> 1034.53
5.2    <-> 1068.93
5.3    <-> 1103.05
@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser May 23, 2017

Contributor

@appukuttan-shailesh The phase is as expected: at 100 Hz, the period is 10 ms, so at 5 ms the sine should cross zero in the negative direction. This looked different previously, because then the sine was not iterated from zero, but only while the device was active.

Contributor

heplesser commented May 23, 2017

@appukuttan-shailesh The phase is as expected: at 100 Hz, the period is 10 ms, so at 5 ms the sine should cross zero in the negative direction. This looked different previously, because then the sine was not iterated from zero, but only while the device was active.

@appukuttan-shailesh

This comment has been minimized.

Show comment
Hide comment
@appukuttan-shailesh

appukuttan-shailesh May 23, 2017

Contributor

Ah ok, I now get what you meant by "slightly different logic than apparently assumed by PyNN ...". Out of curiosity... is there an advantage in implementing it like this?

On the downside, I would imagine that if a modeler wished to inject current in a model sometime during a run, this implementation would require him to be aware of the phase at that particular time instant? Whereas if the sinusoid is computed from the time instant it is activated (the earlier implementation?), this would be more predictable.

Contributor

appukuttan-shailesh commented May 23, 2017

Ah ok, I now get what you meant by "slightly different logic than apparently assumed by PyNN ...". Out of curiosity... is there an advantage in implementing it like this?

On the downside, I would imagine that if a modeler wished to inject current in a model sometime during a run, this implementation would require him to be aware of the phase at that particular time instant? Whereas if the sinusoid is computed from the time instant it is activated (the earlier implementation?), this would be more predictable.

@gtrensch

This comment has been minimized.

Show comment
Hide comment
@gtrensch

gtrensch May 23, 2017

Contributor

@heplesser I have a question: Does the parameter 'origin' should repeat the sequence within 'start' and 'stop' ? If so, the phase needs to be shifted.

Contributor

gtrensch commented May 23, 2017

@heplesser I have a question: Does the parameter 'origin' should repeat the sequence within 'start' and 'stop' ? If so, the phase needs to be shifted.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser May 24, 2017

Contributor

See #505 for some thoughts on this matter in general. Current NEST logic says that start/stop/origin provide strictly a windowing mechanism. As I point out in #505, this mechanism seems dated, and the transition to NEST 3 would provide an opportunity to replace it with something better. I would propose a two-pronged approach:

  • Fixing the errors in the current implementation by merging this PR.
  • Discussing in #505 the syntax and semantics of an improved approach. Especially for repeated stimulation, we need to think carefully about how to handle repeated stimulation.

Just some things to think about: For a sine current one could (as one option) define a fixed phase at the beginning of each repetition and implement this easily. For Poisson and gamma processes etc one needs to handle the first interval properly, and other stimulators may raise yet more issues.

Contributor

heplesser commented May 24, 2017

See #505 for some thoughts on this matter in general. Current NEST logic says that start/stop/origin provide strictly a windowing mechanism. As I point out in #505, this mechanism seems dated, and the transition to NEST 3 would provide an opportunity to replace it with something better. I would propose a two-pronged approach:

  • Fixing the errors in the current implementation by merging this PR.
  • Discussing in #505 the syntax and semantics of an improved approach. Especially for repeated stimulation, we need to think carefully about how to handle repeated stimulation.

Just some things to think about: For a sine current one could (as one option) define a fixed phase at the beginning of each repetition and implement this easily. For Poisson and gamma processes etc one needs to handle the first interval properly, and other stimulators may raise yet more issues.

@gtrensch

This comment has been minimized.

Show comment
Hide comment
@gtrensch

gtrensch May 24, 2017

Contributor

@heplesser thanks, this sounds reasonable.
I approve this PR.
Note: To complete the story from #663 and continued in #720 another PR is required.

Contributor

gtrensch commented May 24, 2017

@heplesser thanks, this sounds reasonable.
I approve this PR.
Note: To complete the story from #663 and continued in #720 another PR is required.

@appukuttan-shailesh

My test simulations seemed to run fine for various combination of inputs. So all clear from me.

As you have suggested, the decision on start/stop/origin might need further discussion and could be continued elsewhere.

p.s. As for the changes in #720 , I suppose I should wait for this PR to go through, and then add changes on top of it (as some of the same files are involved).

@gtrensch

This comment has been minimized.

Show comment
Hide comment
@gtrensch

gtrensch May 24, 2017

Contributor

@appukuttan-shailesh yes, if you merge with master then, you perhaps may get a conflict in ac_generator.cpp.

Contributor

gtrensch commented May 24, 2017

@appukuttan-shailesh yes, if you merge with master then, you perhaps may get a conflict in ac_generator.cpp.

@appukuttan-shailesh

This comment has been minimized.

Show comment
Hide comment
@appukuttan-shailesh

appukuttan-shailesh May 24, 2017

Contributor

@gtrensch : I was going through the commits again... I believe the changes we made for #720 were restricted to:

  1. models/ac_generator.h
  2. models/dc_generator.h
  3. models/noise_generator.h
  4. models/step_current_generator.h
  5. pynest/nest/tests/test_current_recording_generators.py
  6. models/noise_generator.cpp

So the only potential conflict might be with models/ac_generator.h. And here the changes in #730 seem restricted to the comments at the start of the file. So should go through without conflicts, I imagine. Should I proceed with the PR?

Contributor

appukuttan-shailesh commented May 24, 2017

@gtrensch : I was going through the commits again... I believe the changes we made for #720 were restricted to:

  1. models/ac_generator.h
  2. models/dc_generator.h
  3. models/noise_generator.h
  4. models/step_current_generator.h
  5. pynest/nest/tests/test_current_recording_generators.py
  6. models/noise_generator.cpp

So the only potential conflict might be with models/ac_generator.h. And here the changes in #730 seem restricted to the comments at the start of the file. So should go through without conflicts, I imagine. Should I proceed with the PR?

@gtrensch

This comment has been minimized.

Show comment
Hide comment
@gtrensch

gtrensch May 25, 2017

Contributor

Yes, you are right. It was noise_generator.cpp. Please proceed with your PR.
Many thanks !

Contributor

gtrensch commented May 25, 2017

Yes, you are right. It was noise_generator.cpp. Please proceed with your PR.
Many thanks !

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser May 26, 2017

Contributor

Release note: ac_generator will now compute current as sin( om * t + phi ) with t counted from the beginning of the simulation even when using start/stop.

Contributor

heplesser commented May 26, 2017

Release note: ac_generator will now compute current as sin( om * t + phi ) with t counted from the beginning of the simulation even when using start/stop.

@heplesser heplesser merged commit 9b0c0b4 into nest:master May 26, 2017

1 check passed

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

@heplesser heplesser deleted the heplesser:fix-720-ac-generator branch May 26, 2017

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