Exclude "constant over dt" subexpressions from cycle checking #1223
Conversation
| # With constant over dt, the cycle disappears | ||
| Equations('''dv/dt = (-v + s1)/ (10*ms) : 1 | ||
| s1 = 2 * s2 : 1 | ||
| s2 = s1/2 : 1 (constant over dt)''') |
There was a problem hiding this comment.
This feels very unclear to me how to interpret. If s2 is constant over dt but s1 is not, but s2 depends on s1.... wha? Argh! Seriously though, I don't understand what this is supposed to do, and it makes me think we shouldn't be allowing it.
There was a problem hiding this comment.
This is consistent with our use of "constant over dt" elsewhere and I don't see a reason for not allowing it. It's equivalent to
s2 : 1
together with
...run_regularly('s2 = s1/2')
i.e. it refers to the value of s1 at the beginning of the time step.
There was a problem hiding this comment.
Can you remind me what's the primary use case of constant over dt? This feels wrong to me, and I feel like the run_regularly solution would be better in most cases because it forces people to be explicit about what it means, but if the idea was always to freeze a value with its value at the start of the window (for approximations and numerical integration reasons) then it makes sense.
There was a problem hiding this comment.
I think originally we introduced it for expressions involving things like rand(), but you can also use it e.g. to approximate a slow non-linearity as constant over a time step so that you can use exact integration. That it is called "constant over dt" is not great for all situations. But I prefer it over a generic "variable : unit" in the equations which needs the additional run_regularly operation to make sense.
There was a problem hiding this comment.
I think I'd prefer a new syntax for that use case then, and have cycle checking on for constant over dt and off for the new syntax? maybe something like "force constant over dt"?
There was a problem hiding this comment.
Sorry I'm being a bit annoying, but I'm just worried that we're storing up problems for ourselves here.
There was a problem hiding this comment.
But does cycle checking for a situation where it is not a problem make sense? And wouldn't a new syntax make things even more complicated? Happy to postpone the decision on this PR for a bit. It's not a very important issue and if it involves changing syntax, we should make sure we get it right.
There was a problem hiding this comment.
Yeah let's postpone for the moment.
How about this, we allow cycles but we raise a warning to explain to users what it means, only if they have a cycle? If they use (force constant over dt) we assume they know what they're doing and don't raise a warning. The force makes it clear that you're asking Brian to do something that isn't straightforward and taking responsibility for it.
See #1187