Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
31 changes: 19 additions & 12 deletions src/systems/alias_elimination.jl
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,11 @@ end

Pick the target variable for an alias group. Irreducible variables must remain as
unknowns, so one of them is chosen as the target when the group contains any. Otherwise
the variable with the highest `state_priority` wins. When priorities are tied, prefer
the variable appearing in the most equations: visualization-only variables appear in a
single alias equation, while physics variables appear in many dynamics equations.
the variable with the highest `state_priority` wins; all other group members are
eliminated in favour of the target. When positive priorities are tied, emit a warning and prefer the
variable appearing in the most equations (visualization-only variables appear in a single alias
equation, while physics variables appear in many dynamics equations); if that is also
tied, one of the tied variables is chosen arbitrarily.
"""
function pick_alias_target(
fullvars::Vector{SymbolicT}, group_vars::Vector{Int}, state_priorities, irreducibles::AtomicSetT,
Expand All @@ -118,9 +120,14 @@ function pick_alias_target(
irr_idx === nothing || return group_vars[irr_idx]
max_priority = maximum(Base.Fix1(getindex, state_priorities), group_vars)
candidates = filter(v -> state_priorities[v] == max_priority, group_vars)
if graph !== nothing && length(candidates) > 1
_, target_idx = findmax(v -> length(𝑑neighbors(graph, v)), candidates)
return candidates[target_idx]
if length(candidates) > 1 && max_priority > 0
tied_names = getindex.(Ref(fullvars), candidates)
@warn "Multiple variables in an alias group share the highest state_priority \
($max_priority); choosing alias target by equation count. Tied variables: $tied_names"
if graph !== nothing
max_degree = maximum(v -> length(𝑑neighbors(graph, v)), candidates)
candidates = filter(v -> length(𝑑neighbors(graph, v)) == max_degree, candidates)
end
end
return candidates[1]
end
Expand Down Expand Up @@ -203,7 +210,6 @@ function find_perfect_aliases!(
group_target = Dict{Int, Int}()
sizehint!(group_target, length(members))

is_sticky = (v) -> contains_possibly_indexed_element(irreducibles, fullvars[v]) || state_priorities[v] > 0
is_irreducible_v = (v) -> contains_possibly_indexed_element(irreducibles, fullvars[v])

# Symbolic zero used as substitution target for variables in conflict groups.
Expand Down Expand Up @@ -250,9 +256,10 @@ function find_perfect_aliases!(
group_target[root] = target
target_p = parity[target]
for v in group_vars
# In consistent groups sticky vars (irreducible OR priority>0) other
# than the target stay as unknowns.
if is_sticky(v) || v == target
# In consistent groups, irreducible vars other than the target stay as unknowns.
# All other non-target vars (including those with positive state_priority) are
# eliminated in favour of the highest-priority target.
if is_irreducible_v(v) || v == target
state.always_present[v] = true
continue
end
Expand Down Expand Up @@ -318,8 +325,8 @@ function find_perfect_aliases!(
end
else
target = group_target[parent[v1]]
c1 = is_sticky(v1) ? v1 : target
c2 = is_sticky(v2) ? v2 : target
c1 = is_irreducible_v(v1) ? v1 : target
c2 = is_irreducible_v(v2) ? v2 : target
c1 == c2 && push!(eqs_to_rm, ieq)
end
end
Expand Down
28 changes: 28 additions & 0 deletions test/reduction.jl
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,34 @@ ss = mtkcompile(sys)
@test isequal(only(unknowns(sys)), y)
end

@testset "positive-priority variable is eliminated when target has higher priority" begin
# Both x and y have positive state_priority, but y's is higher.
# Old behaviour: both sticky → neither eliminated.
# New behaviour: x (lower priority) is eliminated as observed in favour of y.
@variables x(t) y(t)
@named sys = System([D(y) ~ -y, x ~ y], t; state_priorities = [x => 1, y => 2])
state = TearingState(sys)
ModelingToolkit.eliminate_perfect_aliases!(state)
@test any(eq -> isequal(eq.lhs, unwrap(x)), state.additional_observed)
@test any(isequal(unwrap(y)), state.fullvars)
@test !any(isequal(unwrap(x)), state.fullvars)
end

@testset "warning is emitted when state_priority is tied across alias group" begin
# x appears in two equations (D(x) ~ -x and x ~ y), y in one (x ~ y),
# so the equation-count heuristic picks x as target — but the priority tie
# (both at 5) must still emit a warning regardless of how the tie is broken.
@variables x(t) y(t)
@named sys = System([D(x) ~ -x, x ~ y], t; state_priorities = [x => 5, y => 5])
state = TearingState(sys)
@test_logs (:warn, r"state_priority") match_mode=:any ModelingToolkit.eliminate_perfect_aliases!(state)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[Runic] reported by reviewdog 🐶

Suggested change
@test_logs (:warn, r"state_priority") match_mode=:any ModelingToolkit.eliminate_perfect_aliases!(state)
@test_logs (:warn, r"state_priority") match_mode = :any ModelingToolkit.eliminate_perfect_aliases!(state)

# Exactly one of the two variables is eliminated as observed.
n_elim = count(
eq -> isequal(eq.lhs, unwrap(x)) || isequal(eq.lhs, unwrap(y)),
state.additional_observed)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[Runic] reported by reviewdog 🐶

Suggested change
state.additional_observed)
state.additional_observed
)

@test n_elim == 1
end

@testset "Perfect aliases do not eliminate irreducible variables" begin
@variables x(t) y(t)
@variables e(t) [irreducible = true]
Expand Down
Loading