Fix #1187: Allow cyclical subexpressions with 'constant over dt' flag

[nishantraghuvanshi]

• Exclude 'constant over dt' subexpressions from cycle detection
• Add test_cyclical_subexpressions() to verify the fix
• These subexpressions are evaluated once per timestep, not recursively
  fixes Allow cyclical subexpressions if marked "constant over dt" #1187

[mstimberg]

Hi @nishantraghuvanshi Thanks for the PR update. Could you please briefly comment on why your last changes were necessary and give a short description of them? Thanks!

[nishantraghuvanshi]

Hi @mstimberg,

The last commit fixes a subexpression ordering issue caused by the initial implementation.

Problem: Setting "constant over dt" subexpressions to have no dependencies broke their ordering.

Fix: Now I:

• Keep all dependencies for proper ordering
• Sort regular subexpressions with strict topsort
• Sort "constant over dt" subexpressions with cycle-tolerant algorithm
• Combine both sorted lists

This allows cycles for "constant over dt" while maintaining correct ordering. Also fixed a test isolation issue.
Do I need to make any changes to the approach?

[mstimberg]

[Sorry, I meant to send this last week, but it was still sitting in one of my browser tabs
This looks pretty good to me. I'm not quite ready to merge it yet, since this is one of these ungrateful issues where we have to spend a lot of time looking into details even though they affect almost no code in practice. What I am wondering is whether this changes anything at all for the order of operations in the no-cycle case, e.g. if there are dependencies between the "constant over dt" and "classical" subexpressions. It would be nice to test this with a few examples to be sure, and also to document this once and for all with some clear rules for users to reason about how and when these subexpressions are calculated.

[nishantraghuvanshi]

Hi @mstimberg,

Thank you for the careful review. You were absolutely right to be concerned about the order of operations.

The Bug

The current implementation separated subexpressions into two groups and sorted them independently:

• Regular subexpressions sorted together
• "Constant over dt" subexpressions sorted together
• Then combined as: [all regular] + [all constant]

This breaks cross-dependencies. Example:

s1 = v + 1 : 1 (constant over dt)
s2 = s1 * 2 : 1  (regular)
s3 = s2 + 5 : 1 (constant over dt)

Old code order: s2 → s1 → s3 (this would cause issues)
Correct order: s1 → s2 → s3

The problem: when s3 is computed in SubexpressionUpdater, it needs s2 (which is inlined). But s2 references s1, which is found as a parameter in the model with its OLD value instead of the newly computed value.

The Fix

I've rewritten _sort_subexpressions() to:

1. Sort ALL subexpressions together (preserves dependencies)
2. If there's a cycle, check if it ONLY involves "constant over dt" subexpressions
   • If yes → allow with cycle-tolerant sort
   • If no → raise ValueError

This preserves the old behavior for non-cyclical cases while still allowing cyclical "constant over dt" subexpressions.
Do you think this approach is feasible?