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

Exclude "constant over dt" subexpressions from cycle checking #1223

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mstimberg
Copy link
Member

See #1187

# 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)''')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I'm being a bit annoying, but I'm just worried that we're storing up problems for ourselves here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants