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.

Sign up for GitHub

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

Jump to bottom

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

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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions brian2/equations/equations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -901,14 +901,18 @@ def _sort_subexpressions(self):
static_deps = {}
for eq in self._equations.values():
if eq.type == SUBEXPRESSION:
static_deps[eq.varname] = [dep for dep in eq.identifiers if
dep in self._equations and
self._equations[dep].type == SUBEXPRESSION]

# "Constant over dt" subexpressions are implemented like parameters
# and can be considered as not depending on anything
if 'constant over dt' in eq.flags:
static_deps[eq.varname] = []
else:
static_deps[eq.varname] = [dep for dep in eq.identifiers if
dep in self._equations and
self._equations[dep].type == SUBEXPRESSION]
try:
sorted_eqs = topsort(static_deps)
except ValueError:
raise ValueError('Cannot resolve dependencies between static '
raise ValueError('Cannot resolve dependencies between '
'equations, dependencies contain a cycle.')

# put the equations objects in the correct order
Expand Down
14 changes: 14 additions & 0 deletions brian2/tests/test_equations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,6 +485,20 @@ def test_extract_subexpressions():
assert constant['s2'].type == SUBEXPRESSION


@pytest.mark.codegen_independent
def test_cyclical_subexpressions():
with pytest.raises(ValueError):
# dependency cycle
Equations('''dv/dt = (-v + s1)/ (10*ms) : 1
s1 = 2 * s2 : 1
s2 = s1/2 : 1''')

# 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.



@pytest.mark.codegen_independent
def test_repeated_construction():
eqs1 = Equations('dx/dt = x : 1')
Expand Down