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 sigmoid growth curve for structural plasticity #476

Merged
merged 11 commits into from Feb 28, 2017

Conversation

Projects
None yet
5 participants
@sanjayankur31

This comment has been minimized.

Show comment
Hide comment
@sanjayankur31

sanjayankur31 Sep 15, 2016

Contributor

@sdiazpier , @heplesser - would you have the time to review this one? :)

Contributor

sanjayankur31 commented Sep 15, 2016

@sdiazpier , @heplesser - would you have the time to review this one? :)

@sdiazpier

This comment has been minimized.

Show comment
Hide comment
@sdiazpier

sdiazpier Sep 15, 2016

Contributor

Hi dear @sanjayankur31 this is very nice, I wanted to add a sigmoid growth curve for some time now. My first quick look gives me the impression that the code is ok. I only see some issues in the format of the parameter descriptions in the documentation of the growth curve header. Could you please take a look at this? I promise to give it a deep review tomorrow and give you a thumbs up if I see no further issues.

Contributor

sdiazpier commented Sep 15, 2016

Hi dear @sanjayankur31 this is very nice, I wanted to add a sigmoid growth curve for some time now. My first quick look gives me the impression that the code is ok. I only see some issues in the format of the parameter descriptions in the documentation of the growth curve header. Could you please take a look at this? I promise to give it a deep review tomorrow and give you a thumbs up if I see no further issues.

@sanjayankur31

This comment has been minimized.

Show comment
Hide comment
@sanjayankur31

sanjayankur31 Sep 15, 2016

Contributor

Thanks! I'll fix up the formatting first thing tomorrow morning :)

Contributor

sanjayankur31 commented Sep 15, 2016

Thanks! I'll fix up the formatting first thing tomorrow morning :)

@sdiazpier

This comment has been minimized.

Show comment
Hide comment
@sdiazpier

sdiazpier Sep 23, 2016

Contributor

Hi dear @sanjayankur31. I think the code is ok.
I have a suggestion though. I see that you implemented the sigmoid function just as described in the paper by Dr. Butz. Nevertheless, there is a parameter which I think could be interesting to add to this curve. It is related to the width of the curve (the distance between the curve reaching 95% of the max and 95% of the min. The equation currently looks like this: y = nu ((2 / (1 + e^((Ca(t) - eps)/0.1))) -1 ). I would propose to change it to y = nu ((2 / (1 + e^((Ca(t) - eps) / w ))) -1 ) where w controls the width of the curve. This is useful because the rate of change at the intersection of the curve with the x axis has a lot of influence in the stability of the system. If the rate of change in dz/dt is too high (the width is small) it is more likely that the number of synaptic elements will oscillate more before stability is reached.
Of course, adding this parameter requires some checks when the creation of the curve is made so that the value is never 0.

Contributor

sdiazpier commented Sep 23, 2016

Hi dear @sanjayankur31. I think the code is ok.
I have a suggestion though. I see that you implemented the sigmoid function just as described in the paper by Dr. Butz. Nevertheless, there is a parameter which I think could be interesting to add to this curve. It is related to the width of the curve (the distance between the curve reaching 95% of the max and 95% of the min. The equation currently looks like this: y = nu ((2 / (1 + e^((Ca(t) - eps)/0.1))) -1 ). I would propose to change it to y = nu ((2 / (1 + e^((Ca(t) - eps) / w ))) -1 ) where w controls the width of the curve. This is useful because the rate of change at the intersection of the curve with the x axis has a lot of influence in the stability of the system. If the rate of change in dz/dt is too high (the width is small) it is more likely that the number of synaptic elements will oscillate more before stability is reached.
Of course, adding this parameter requires some checks when the creation of the curve is made so that the value is never 0.

@sanjayankur31

This comment has been minimized.

Show comment
Hide comment
@sanjayankur31

sanjayankur31 Sep 24, 2016

Contributor

Hi @sdiazpier - makes sense. I'll update the code in a few days. Thanks :)

Contributor

sanjayankur31 commented Sep 24, 2016

Hi @sdiazpier - makes sense. I'll update the code in a few days. Thanks :)

@sanjayankur31

This comment has been minimized.

Show comment
Hide comment
@sanjayankur31

sanjayankur31 Oct 11, 2016

Contributor

Hi @sdiazpier - I've added the new variable. I've used w for the time being, but in nest_names, w is already defined to mean something else. Should I change it?

I'm also not clear on when one needs to add to nest_names and when not. For example, when I'd added the vogels-sprekeler synapse model, I hadn't added things like alpha, eta, or tau to nest_names. Do I need to ensure that these params and w are in nest_names?

Thanks :)

Contributor

sanjayankur31 commented Oct 11, 2016

Hi @sdiazpier - I've added the new variable. I've used w for the time being, but in nest_names, w is already defined to mean something else. Should I change it?

I'm also not clear on when one needs to add to nest_names and when not. For example, when I'd added the vogels-sprekeler synapse model, I hadn't added things like alpha, eta, or tau to nest_names. Do I need to ensure that these params and w are in nest_names?

Thanks :)

@sdiazpier

This comment has been minimized.

Show comment
Hide comment
@sdiazpier

sdiazpier Oct 26, 2016

Contributor

Hi dear @sanjayankur31 sorry for the delay. Adding the params to nest_names helps maintainability and consistency when using and extending the code, so the best would be to find another suitable name for the variable w. Maybe it could be changed to psi.
For the vogels-sprekeler model, it would also be good to define the param names in nest_names in case these parameters are specific to your model.

Contributor

sdiazpier commented Oct 26, 2016

Hi dear @sanjayankur31 sorry for the delay. Adding the params to nest_names helps maintainability and consistency when using and extending the code, so the best would be to find another suitable name for the variable w. Maybe it could be changed to psi.
For the vogels-sprekeler model, it would also be good to define the param names in nest_names in case these parameters are specific to your model.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Oct 31, 2016

Contributor

@apeyser Do you think you could take a look at this one?

Contributor

heplesser commented Oct 31, 2016

@apeyser Do you think you could take a look at this one?

@sanjayankur31

This comment has been minimized.

Show comment
Hide comment
@sanjayankur31

sanjayankur31 Nov 2, 2016

Contributor

Working on this now. Will hopefully push new commits before the weekend. :)

Contributor

sanjayankur31 commented Nov 2, 2016

Working on this now. Will hopefully push new commits before the weekend. :)

@sdiazpier

This comment has been minimized.

Show comment
Hide comment
@sdiazpier

sdiazpier Dec 14, 2016

Contributor

Dear @sanjayankur31 sorry for the delay to respond. Could you please merge the latest changes in nest_names.h? For the rest the PR looks ready for me. 👍
Thanks for adding this nice functionality and for your active work on structural plasticity and nest.

Contributor

sdiazpier commented Dec 14, 2016

Dear @sanjayankur31 sorry for the delay to respond. Could you please merge the latest changes in nest_names.h? For the rest the PR looks ready for me. 👍
Thanks for adding this nice functionality and for your active work on structural plasticity and nest.

@sanjayankur31

This comment has been minimized.

Show comment
Hide comment
@sanjayankur31

sanjayankur31 Dec 14, 2016

Contributor

dear @sdiazpier - I've resolved the conflicts and merged with master. Please do let me know if there's anything else :)

Contributor

sanjayankur31 commented Dec 14, 2016

dear @sdiazpier - I've resolved the conflicts and merged with master. Please do let me know if there's anything else :)

@sanjayankur31

This comment has been minimized.

Show comment
Hide comment
@sanjayankur31

sanjayankur31 Jan 12, 2017

Contributor

@sdiazpier - updated to remove conflicts.

Contributor

sanjayankur31 commented Jan 12, 2017

@sdiazpier - updated to remove conflicts.

@terhorstd terhorstd requested review from flinz and sdiazpier Jan 23, 2017

@sdiazpier

Great job @sanjayankur31. Nothing more to add from my side.

@sanjayankur31

This comment has been minimized.

Show comment
Hide comment
@sanjayankur31

sanjayankur31 Feb 20, 2017

Contributor

I've corrected the conflicts. Please let me know if there are any more changes required. Cheers!

Contributor

sanjayankur31 commented Feb 20, 2017

I've corrected the conflicts. Please let me know if there are any more changes required. Cheers!

@flinz

Good job, this is already quite polished!
👍 from me once the single typo i found is addressed!

Show outdated Hide outdated pynest/nest/tests/test_sp/test_growth_curves.py
"""
Compute the number of synaptic element corresponding to a
linear growth curve

This comment has been minimized.

@flinz

flinz Feb 28, 2017

Contributor

Should this be sigmoid/sigmoidal growth curve, instead of linear?

@flinz

flinz Feb 28, 2017

Contributor

Should this be sigmoid/sigmoidal growth curve, instead of linear?

@flinz

This comment has been minimized.

Show comment
Hide comment
@flinz

flinz Feb 28, 2017

Contributor

from me 👍
thanks for your work!

Contributor

flinz commented Feb 28, 2017

from me 👍
thanks for your work!

@sanjayankur31

This comment has been minimized.

Show comment
Hide comment
@sanjayankur31

sanjayankur31 Feb 28, 2017

Contributor

@flinz - thank you for the review!

Contributor

sanjayankur31 commented Feb 28, 2017

@flinz - thank you for the review!

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Feb 28, 2017

Contributor

@jougs This is ready to merge, I leave it to you whether you want to include it with NEST 2.12.

Contributor

heplesser commented Feb 28, 2017

@jougs This is ready to merge, I leave it to you whether you want to include it with NEST 2.12.

@terhorstd terhorstd merged commit 773cb1c into nest:master Feb 28, 2017

1 check passed

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

@sanjayankur31 sanjayankur31 deleted the sanjayankur31:growth-curve-sigmoid branch Feb 28, 2017

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