Skip to content

Qualify clock operators in subsystem isolation - #4988

Closed
ChrisRackauckas-Claude wants to merge 1 commit into
SciML:masterfrom
ChrisRackauckas-Claude:agent/fix-interfaceii-operator-owner
Closed

Qualify clock operators in subsystem isolation#4988
ChrisRackauckas-Claude wants to merge 1 commit into
SciML:masterfrom
ChrisRackauckas-Claude:agent/fix-interfaceii-operator-owner

Conversation

@ChrisRackauckas-Claude

Copy link
Copy Markdown
Member

Ignore this PR until it has been reviewed by @ChrisRackauckas.

What changed

isolate_subsystem now recognizes ModelingToolkit's clock operators through their public owning modules: Symbolics.Differential and ModelingToolkitBase.{Shift,Sample,Hold,SampleTime}. This restores clock propagation without reintroducing the accidental module-global Operator binding or depending on the non-public SymbolicUtils.Operator type.

The existing Clock information inferred from removed sections is retained regression test is the discriminator; no duplicate test was added.

Root cause and bisect

The clock-retention implementation introduced an unqualified Operator reference in commit 313cc258c84544a5d32a8c487b8f40bf8371c174. It happened to resolve through using SymbolicUtils: Operator in another included source file.

Commit 03c130782349ca8ed91e4f9a92da08d590c53ffe correctly removed that incidental import while making dependency imports owner-qualified. Its parent, 64ced006057dfef83d26f8941767c2a74653e1e6, reports isdefined(ModelingToolkit, :Operator) == true locally and had green InterfaceII jobs on Julia LTS/current/pre. Clean current master and both PR routes fail with the same UndefVarError on all three Julia versions.

An API audit on Julia 1.12 confirmed that all five concrete types used here are public and exported by their owning modules. Base.ispublic(SymbolicUtils, :Operator) and Base.isexported(SymbolicUtils, :Operator) are both false, so qualifying the old abstract type as SU.Operator would still depend on an internal API.

Verification

Failing before, on clean master SHA 3fe375e19031102e95c7775c4d8a6aac9895767b:

GROUP=InterfaceII julia +1.12 --startup-file=no --project=. -e 'using Pkg; Pkg.test()'

Clock information inferred from removed sections is retained: Error During Test
  UndefVarError: `Operator` not defined in `ModelingToolkit`
  isolate_subsystem at src/systems/analysis_points.jl:301
  test at lib/ModelingToolkitBase/test/analysis_points.jl:1213

Passing after:

GROUP=InterfaceII julia +1.12 --startup-file=no --project=. -e 'using Pkg; Pkg.test()'
Test Summary: | Pass  Broken  Total      Time
InterfaceII   | 1038       7   1045  53m19.7s
Testing ModelingToolkit tests passed

GROUP=QA julia +1.12 --startup-file=no --project=. -e 'using Pkg; Pkg.test()'
Test Summary: | Pass  Total     Time
QA            |   52     52  4m27.0s
Testing ModelingToolkit tests passed

After rebasing onto 29f589e204ca80cfe550af04c49a6f3010bf92d5 (which only changed three InterfaceI test files), the standalone form of the same clock-retention reproducer also passed:

regression pass: SciMLBase.AbstractClock[SciMLBase.PeriodicClock(0.1, 0.0), SciMLBase.PeriodicClock(0.1, 0.0)]

Additional gates:

Runic 1.8.0 --check --diff src/systems/analysis_points.jl  # exit 0
typos src/systems/analysis_points.jl                       # exit 0
git diff --check                                           # exit 0

Documentation was not built because this changes neither documentation nor public API. Julia LTS and pre-release were not run locally; CI covers those lanes.

Links

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
@ChrisRackauckas

Copy link
Copy Markdown
Member

Instead make Operator public.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants