Skip to content

Fix symbolic option parameter discovery#1503

Draft
ChrisRackauckas-Claude wants to merge 3 commits into
SciML:latest_scimlbase_difeq_adaptionfrom
ChrisRackauckas-Claude:fix-pr1488-modeling-lts
Draft

Fix symbolic option parameter discovery#1503
ChrisRackauckas-Claude wants to merge 3 commits into
SciML:latest_scimlbase_difeq_adaptionfrom
ChrisRackauckas-Claude:fix-pr1488-modeling-lts

Conversation

@ChrisRackauckas-Claude

Copy link
Copy Markdown
Member

This PR targets #1488 (latest_scimlbase_difeq_adaption).

Please ignore until reviewed by @ChrisRackauckas.

Summary

  • Discover parameters and unknowns from symbolic option expressions using Symbolics.get_variables instead of relying on MT.collect_vars! for bare expressions.
  • Apply the same discovery path to observed RHS expressions, @tstops, and poissonian rate expressions.

Verification

  • /home/crackauc/.juliaup/bin/julia +1.10 --project=. -e 'using Pkg; Pkg.test()' with temporary runner narrowed to DSL Options: 317 passed, 1 broken.\n- GROUP=Modeling /home/crackauc/.juliaup/bin/julia +1.10 --project=. -e 'using Pkg; Pkg.test()': Catalyst tests passed.\n- git diff --check: passed.\n- Runic.main(["--check", "--diff", "--lines=198:232", "--lines=249:252", "--lines=670:680", "src/reactionsystem.jl"]): passed for changed ranges.\n

@isaacsas

isaacsas commented Jul 8, 2026

Copy link
Copy Markdown
Member

What is the purpose of this? Stopping using the ModelingToolit function and instead rolling our own function is exactly the kind of thing that subsequently causes breakage.

@ChrisRackauckas

Copy link
Copy Markdown
Member

This is GPT gone rogue 😅 I'm not sure which agent did this... I didn't have any pointed at Catalyst.jl. Curious...

AayushSabharwal and others added 3 commits July 8, 2026 19:03
Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
@TorkelE
TorkelE force-pushed the latest_scimlbase_difeq_adaption branch from e2c4ed8 to 84b29ff Compare July 12, 2026 08:41
@ChrisRackauckas-Claude

Copy link
Copy Markdown
Member Author

Independent clean-base regression audit on Catalyst ae55024:

  • The repository harness reproduces the exact failure locally on Julia 1.10.11 with current resolved dependencies:

    GROUP=Modeling JULIA_NUM_THREADS=auto JULIA_PKG_PRECOMPILE_AUTO=0 julia +1.10 --project=. --check-bounds=yes --depwarn=yes -e 'using Pkg; Pkg.test()'
    

    Result: DSL Options | 308 pass, 9 fail, 1 broken, 318 total. The failures are the same assertions at dsl_options.jl:793, 1187, 1260, 1274, 1287, 1303, 1956, 1974, and 1992.

  • Historical CI isolates the dependency release boundary. The June 28 job passed DSL Options with SymbolicUtils 4.37.1, while the June 29 job failed those exact nine assertions with SymbolicUtils 4.37.2. ModelingToolkitBase 1.48.1, OrdinaryDiffEq 7.1.1, StochasticDiffEq 7.1.1, and Symbolics 7.29.0 were identical between the two jobs:

  • A reduced Julia 1.10 load-order reproducer points at the existing ModelingToolkitBase collect_vars! traversal. Without loading Plots, the programmatic ReactionSystem constructor discovers [k, k_obs, k_ratio, eps_val]. With Plots loaded first, the constructor discovers only [k], and a subsequent direct collect_vars! traversal of an observed RHS returns empty sets. Reversing the order lets the first direct traversal work, but the subsequent constructor discovers only the observed parameters and misses reaction parameter k.

  • The v4.37.1..v4.37.2 SymbolicUtils production source window is the single expression-ordering PR fix: make expression ordering less dependent on hash JuliaSymbolics/SymbolicUtils.jl#990. Its first production commit is f4f5ccc5c67302c5c647517cb97727b35c36da63; the same PR also contains the later comparator refactor 8bffce2e.

This is consistent with the Julia 1.10 collect_vars! compiler/load-order behavior previously reported in ModelingToolkit.jl#4211. The local collector in this PR can mask the symptom by changing compilation order, but it duplicates upstream variable-discovery semantics. I recommend retaining a Catalyst downstream regression test while fixing the existing public ModelingToolkitBase collect_vars! path, rather than merging the package-local replacement. The upstream design discussion is in SciML/ModelingToolkit.jl#4714.

@ChrisRackauckas-Claude

Copy link
Copy Markdown
Member Author

Follow-up: the same-machine exact-stack A/B is complete on Julia 1.10.11.

Pinned stack for both runs: ModelingToolkitBase 1.48.1, OrdinaryDiffEq 7.1.1, StochasticDiffEq 7.1.1, Symbolics 7.29.0, and Plots 1.41.6. The full import order was Catalyst, ModelingToolkitBase, OrdinaryDiffEq, Statistics, StochasticDiffEq, Plots, Test, StableRNGs.

With SymbolicUtils 4.37.1:

CONSTRUCTOR_PARAMETERS=[k, k_obs, k_ratio, eps_val]
COMPLETE_PARAMETERS=[k, k_obs, k_ratio, eps_val]
POST_CONSTRUCTOR_DIRECT_US=[A(t), B(t)]
POST_CONSTRUCTOR_DIRECT_PS=[k_obs]
PARAMETER_ASSERTION=true

I then ran Pkg.add(PackageSpec(name="SymbolicUtils", version=v"4.37.2"); preserve=Pkg.PRESERVE_ALL). Pkg reported only SymbolicUtils 4.37.1 => 4.37.2; the other pinned versions were unchanged. Re-running the identical program produced:

CONSTRUCTOR_PARAMETERS=[k]
COMPLETE_PARAMETERS=[k]
POST_CONSTRUCTOR_DIRECT_US=[]
POST_CONSTRUCTOR_DIRECT_PS=[]
PARAMETER_ASSERTION=false

This directly confirms the 4.37.2 dependency boundary locally, in addition to the historical CI pair.

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.

4 participants