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.

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

Allow negative exponential distributions (fixes #835) #836

Merged
merged 4 commits into from Feb 6, 2018
Merged

Conversation

@luis-rr
Copy link
Contributor

@luis-rr luis-rr commented Oct 4, 2017

Some models rely on the synaptic weight being negative in order to consider the synapse inhibitory.
However the exponential distribution wasn't allowing a negative lambda value.
With this change we can generate exponential distributions of inhibitory synaptic weights.

Note that the implementation in librandom/exp_randomdev.h already works for negative lambda values, this is just removing the check for negative parameter values.

Luis Riquelme
Some models rely on the synaptic weight being negative in order to consider the synapse inhibitory.
However the exponential distribution wasn't allowing a negative lambda value.
With this change we can generate exponential distributions of inhibitory synaptic weights.

Note that the implementation in librandom/exp_randomdev.h already works for negative lambda values.
Copy link
Contributor

@heplesser heplesser left a comment

@luis-rr Thank you for your PR! Your changes look fine, but the documentation needs to be updated to reflect that negative lambda are now allowed (in the header file, |lambda| in the exponential).

@heplesser heplesser changed the title Allow negative exponential distributions Allow negative exponential distributions (fixes #835) Oct 6, 2017
@heplesser
Copy link
Contributor

@heplesser heplesser commented Oct 6, 2017

Please also send me a signed contributor license agreement (see http://nest.github.io/nest-simulator/index) by direct email.

Luis Riquelme
@@ -40,7 +40,9 @@ namespace librandom
Name: rdevdict::exponential - exponential random deviate generator
Description: Generates exponentially distributed random numbers.
p(x) = lambda exp(-lambda*x), x >= 0.
p(x) = lambda exp(-lambda*x), x >= 0 if lambda > 0
p(x) = -lambda exp(-lambda*x), x <= 0 if lambda < 0

This comment has been minimized.

@heplesser

heplesser Oct 20, 2017
Contributor

I would write this differently for clarity: Add a sentence above pointing out explicitly that negative lambda are permitted and then yield negative numbers, and then

For lambda < 0:
  p(x) = 0 for x >= 0
  p(x) = |lambda| exp ( -|lambda| |x| ) for x < 0

Since we are coercing the usual notation here, we should be pretty explicit. Ideally, we would want to draw a positive x and then multiply that with -1, but that will only become possible in the future (see #488 for ideas under consideration).

@@ -39,8 +39,15 @@ namespace librandom
/*BeginDocumentation
Name: rdevdict::exponential - exponential random deviate generator
Description: Generates exponentially distributed random numbers.
Negative values of lambda are allowed and generate a distribution of negative numbers.

This comment has been minimized.

@heplesser

heplesser Oct 24, 2017
Contributor

@luis-rr This line is longer than 79 characters, so it fails our code formatting checks. Could you fix it?

This comment has been minimized.

@luis-rr

luis-rr Oct 25, 2017
Author Contributor

Just sent another commit.

Out of curiosity, how did you identify the error? I'm looking at Travis' log and all I can see is:
MSGBLD0220: C/C++ files with formatting errors:
MSGBLD0220: ... librandom/exp_randomdev.cpp
MSGBLD0220: ... librandom/exp_randomdev.h

But not the description of the formatting error itself.

Copy link
Contributor

@heplesser heplesser left a comment

The code is fine now, just one little formatting issue to fix to make Travis happy. Thanks!

Luis Riquelme
@jougs
jougs approved these changes Feb 6, 2018
Copy link
Contributor

@jougs jougs left a comment

Thanks for the contribution and sorry for the long delay. I'm merging without further ado.

@jougs jougs merged commit a7ffee9 into nest:master Feb 6, 2018
1 check passed
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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.