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

Check monitored expressions for stateful functions #721

Closed
mstimberg opened this Issue Jun 20, 2016 · 5 comments

Comments

Projects
None yet
2 participants
@mstimberg
Member

mstimberg commented Jun 20, 2016

The following is a not uncommon mistake:

group = NeuronGroup(1, '''dv/dt = (-v + I) / (10*ms) : 1
                          I = rand() : 1''')
mon = StateMonitor(group, 'I', record=True)

The user expects that the StateMonitor records the same I that is used in the differential equations, but instead rand() will be evaluated twice, giving different values. We should detect this situation (the most general check seems to be to check for subexpressions involving "stateful functions" ) and raise a warning (or even error?). Alternatively, we could completely forbid to record subexpressions, but that would be quite annoying for all the cases where it is unproblematic.

The warning/error message should direct the user to the use of a parameter and run_regularly, or some new syntax as discussed in #720.

@thesamovar

This comment has been minimized.

Show comment
Hide comment
@thesamovar

thesamovar Jun 20, 2016

Member

Could there be a case where the user would expect I to be the same value in two different places without recording? e.g. with a linked variable or some other mechanism? If not, I'd be happy either to raise a warning or to ban recording of subexpressions with stateful functions in them. I also wouldn't forbid recording subexpressions: seems like overkill.

Member

thesamovar commented Jun 20, 2016

Could there be a case where the user would expect I to be the same value in two different places without recording? e.g. with a linked variable or some other mechanism? If not, I'd be happy either to raise a warning or to ban recording of subexpressions with stateful functions in them. I also wouldn't forbid recording subexpressions: seems like overkill.

@thesamovar

This comment has been minimized.

Show comment
Hide comment
@thesamovar

thesamovar Jun 20, 2016

Member

How does I=rand():1 work with multi-step integration methods btw? Does it use multiple rand() calls or just one? If we haven't already documented that - it might be worth doing too.

Member

thesamovar commented Jun 20, 2016

How does I=rand():1 work with multi-step integration methods btw? Does it use multiple rand() calls or just one? If we haven't already documented that - it might be worth doing too.

@mstimberg

This comment has been minimized.

Show comment
Hide comment
@mstimberg

mstimberg Jun 20, 2016

Member

Good points, you are right that there is nothing monitor-specific here, it is really about the use of stateful functions in a subexpression. I think our current behaviour related to this is inconsistent, IIRC (I did not double check) we substitute all subexpressions for the state update step but only evaluate them once in general code blocks (e.g. resets), with some logic of re-evaluating them if some of the variables they used change. Not sure we added anything to re-evaluate them unconditionally if they involve stateless functions.

I think one fundamental problem in our syntax is that subexpressions look like statements but they are interpreted as functions of their variables.

Member

mstimberg commented Jun 20, 2016

Good points, you are right that there is nothing monitor-specific here, it is really about the use of stateful functions in a subexpression. I think our current behaviour related to this is inconsistent, IIRC (I did not double check) we substitute all subexpressions for the state update step but only evaluate them once in general code blocks (e.g. resets), with some logic of re-evaluating them if some of the variables they used change. Not sure we added anything to re-evaluate them unconditionally if they involve stateless functions.

I think one fundamental problem in our syntax is that subexpressions look like statements but they are interpreted as functions of their variables.

@mstimberg

This comment has been minimized.

Show comment
Hide comment
@mstimberg

mstimberg Jun 20, 2016

Member

One more related remark: if we come up with some syntax for #720, then I think the cleanest would be to raise an error if the subexpression involves stateful functions and force the user to make an explicit choice for that expression, somewhat similar to clock-driven vs. event-driven for synapses. This way, you cannot simply ignore/overlook the warning, but you also have a way to make the warning disappear.
Also needs some though what that means for subexpressions referring to other subexpressions.

Member

mstimberg commented Jun 20, 2016

One more related remark: if we come up with some syntax for #720, then I think the cleanest would be to raise an error if the subexpression involves stateful functions and force the user to make an explicit choice for that expression, somewhat similar to clock-driven vs. event-driven for synapses. This way, you cannot simply ignore/overlook the warning, but you also have a way to make the warning disappear.
Also needs some though what that means for subexpressions referring to other subexpressions.

@thesamovar

This comment has been minimized.

Show comment
Hide comment
@thesamovar

thesamovar Jun 20, 2016

Member

Agreed, see comments on #720.

We should remember to consider the issue of re-evaluating them if they're marked as non-constant during a time step.

Member

thesamovar commented Jun 20, 2016

Agreed, see comments on #720.

We should remember to consider the issue of re-evaluating them if they're marked as non-constant during a time step.

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