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

PoissonGroup's interpretation of string expressions is very confusing #660

Closed
mstimberg opened this Issue Apr 6, 2016 · 7 comments

Comments

Projects
None yet
3 participants
@mstimberg
Member

mstimberg commented Apr 6, 2016

PoissonGroup allows a string expression for the rates, but it evaluates this expression when it is constructed, not at every time step. This is not what the user expects (see the mailing list: https://groups.google.com/d/topic/briansupport/uWXO31OIwDU/discussion), especially for something like rates='timed_array(t)', which will be evaluated at the current time and then used as a constant rate...

It should be very easy to replace the parameter rates : Hz used in the PoissonGroup by a subexpression rates = user_expression : Hz that does what the user expects. At the absolute minimum, the documentation should mention the fact that it does not work as one might expect.

@mstimberg mstimberg added bug easy labels Apr 6, 2016

@mstimberg

This comment has been minimized.

Show comment
Hide comment
@mstimberg

mstimberg Apr 28, 2016

Member

Implementing the change I described above would change the semantics, do we have to do something about this (e.g. a warning, or have it in a separate argument or something like that)? I'd rather say no, I think everyone expects it to work like the proposed approach already and the previous interpretation can simply be considered a bug.

Member

mstimberg commented Apr 28, 2016

Implementing the change I described above would change the semantics, do we have to do something about this (e.g. a warning, or have it in a separate argument or something like that)? I'd rather say no, I think everyone expects it to work like the proposed approach already and the previous interpretation can simply be considered a bug.

@thesamovar

This comment has been minimized.

Show comment
Hide comment
@thesamovar

thesamovar Apr 28, 2016

Member

I think it's OK. I would also have assumed it would be every timestep. One thing though, it would be nice to also support the existing mechanism (because why re-evaluate every time step if it's not changing). Perhaps we could just check for certain scenarios though, like if the expression doesn't depend on time or any other variables?

Member

thesamovar commented Apr 28, 2016

I think it's OK. I would also have assumed it would be every timestep. One thing though, it would be nice to also support the existing mechanism (because why re-evaluate every time step if it's not changing). Perhaps we could just check for certain scenarios though, like if the expression doesn't depend on time or any other variables?

@mstimberg

This comment has been minimized.

Show comment
Hide comment
@mstimberg

mstimberg May 2, 2016

Member

One thing though, it would be nice to also support the existing mechanism (because why re-evaluate every time step if it's not changing). Perhaps we could just check for certain scenarios though, like if the expression doesn't depend on time or any other variables?

I am not sure that is worth the effort. Can you think of any scenario where the evaluation of the expression takes any meaningful amount of time? The PoissonGroup does not have state variables of its own, so the expression is almost necessarily scalar (the calculation could refer to i, but I am not even sure users are aware of that). The most complicated but somewhat realistic scenario I could come up with was something like that:

max_freq = 100*Hz
preferred = 0*pi
stimulus = 0.5*pi

group = PoissonGroup(1000, rates='sin(stimulus-preferred)*max_freq')

Now most users would probably not write the rates part as a string but instead just write it down as a Python expression, so there wouldn't be any issue in the first place. But even if our mechanism always transformed a string expression into a rates subexpression, then this would be the equivalent to

group = NeuronGroup(1000, 'rate = sin(stimulus-preferred)*max_freq : Hz',
                     threshold='rand() < rate*dt')

and in a quick comparison, I did not see any difference in the timing for both cases (in the standalone the latter should even be faster, since the expression can be evaluated at compile-time). Even if it is not scalar (e.g. if I replace max_freq by i*Hz), I never managed to find a performance difference of more than 7% for the thresholder.

So I don't think we should not bother with any kind of (potentially error-prone) analysis of the expression and just always use a subexpression when the rates parameter is a string. What do you think?

Member

mstimberg commented May 2, 2016

One thing though, it would be nice to also support the existing mechanism (because why re-evaluate every time step if it's not changing). Perhaps we could just check for certain scenarios though, like if the expression doesn't depend on time or any other variables?

I am not sure that is worth the effort. Can you think of any scenario where the evaluation of the expression takes any meaningful amount of time? The PoissonGroup does not have state variables of its own, so the expression is almost necessarily scalar (the calculation could refer to i, but I am not even sure users are aware of that). The most complicated but somewhat realistic scenario I could come up with was something like that:

max_freq = 100*Hz
preferred = 0*pi
stimulus = 0.5*pi

group = PoissonGroup(1000, rates='sin(stimulus-preferred)*max_freq')

Now most users would probably not write the rates part as a string but instead just write it down as a Python expression, so there wouldn't be any issue in the first place. But even if our mechanism always transformed a string expression into a rates subexpression, then this would be the equivalent to

group = NeuronGroup(1000, 'rate = sin(stimulus-preferred)*max_freq : Hz',
                     threshold='rand() < rate*dt')

and in a quick comparison, I did not see any difference in the timing for both cases (in the standalone the latter should even be faster, since the expression can be evaluated at compile-time). Even if it is not scalar (e.g. if I replace max_freq by i*Hz), I never managed to find a performance difference of more than 7% for the thresholder.

So I don't think we should not bother with any kind of (potentially error-prone) analysis of the expression and just always use a subexpression when the rates parameter is a string. What do you think?

@thesamovar

This comment has been minimized.

Show comment
Hide comment
@thesamovar

thesamovar May 2, 2016

Member

Great analysis - I'm convinced!

Member

thesamovar commented May 2, 2016

Great analysis - I'm convinced!

mstimberg added a commit that referenced this issue May 2, 2016

mstimberg added a commit that referenced this issue May 3, 2016

@mstimberg mstimberg self-assigned this May 3, 2016

@mstimberg mstimberg closed this in #690 May 4, 2016

@mstimberg mstimberg removed the in progress label May 4, 2016

@flinz

This comment has been minimized.

Show comment
Hide comment
@flinz

flinz Nov 3, 2016

Contributor

Hi, it would be great if this feature was documented a little more carefully (maybe by example) in the https://brian2.readthedocs.io/en/stable/user/input.html#poisson-inputs and https://brian2.readthedocs.io/en/stable/reference/brian2.input.poissongroup.PoissonGroup.html#brian2.input.poissongroup.PoissonGroup.

Maybe extending the text (https://brian2.readthedocs.io/en/stable/user/input.html#poisson-inputs) with some TimedArray example (as in https://github.com/brian-team/brian2/blob/master/brian2/tests/test_poissongroup.py#L36):

If rates is a string, then it is equivalent to NeuronGroup(N, 'rates = ... Hz', threshold='rand()<rates*dt') with the respective expression for the rates (which will be evaluated at every time step and therefore allows time-dependent rates).

Contributor

flinz commented Nov 3, 2016

Hi, it would be great if this feature was documented a little more carefully (maybe by example) in the https://brian2.readthedocs.io/en/stable/user/input.html#poisson-inputs and https://brian2.readthedocs.io/en/stable/reference/brian2.input.poissongroup.PoissonGroup.html#brian2.input.poissongroup.PoissonGroup.

Maybe extending the text (https://brian2.readthedocs.io/en/stable/user/input.html#poisson-inputs) with some TimedArray example (as in https://github.com/brian-team/brian2/blob/master/brian2/tests/test_poissongroup.py#L36):

If rates is a string, then it is equivalent to NeuronGroup(N, 'rates = ... Hz', threshold='rand()<rates*dt') with the respective expression for the rates (which will be evaluated at every time step and therefore allows time-dependent rates).

@mstimberg

This comment has been minimized.

Show comment
Hide comment
@mstimberg

mstimberg Nov 4, 2016

Member

Hi @flinz. I agree that this should be documented better... The docstring is ok-ish, I guess, it at least mentions it explicitly in the documentation of the rates argument. But the main user documentation definitely needs an update. Could I maybe interest you in opening a pull request with such a change (the file to edit is docs_sphinx/user/input.rst)? It'd forever inscribe you into the Brian2 contributor hall of fame 😄

Member

mstimberg commented Nov 4, 2016

Hi @flinz. I agree that this should be documented better... The docstring is ok-ish, I guess, it at least mentions it explicitly in the documentation of the rates argument. But the main user documentation definitely needs an update. Could I maybe interest you in opening a pull request with such a change (the file to edit is docs_sphinx/user/input.rst)? It'd forever inscribe you into the Brian2 contributor hall of fame 😄

@flinz

This comment has been minimized.

Show comment
Hide comment
@flinz

flinz Nov 7, 2016

Contributor

@mstimberg its probably not hall of fame worthy, but I added a minimal example. If you have any idea/wishes of how this could be extended, please mention in the PR.

Contributor

flinz commented Nov 7, 2016

@mstimberg its probably not hall of fame worthy, but I added a minimal example. If you have any idea/wishes of how this could be extended, please mention in the PR.

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