Skip to content

Add expression variable collector#4714

Draft
ChrisRackauckas-Claude wants to merge 1 commit into
SciML:masterfrom
ChrisRackauckas-Claude:add-expression-var-collector
Draft

Add expression variable collector#4714
ChrisRackauckas-Claude wants to merge 1 commit into
SciML:masterfrom
ChrisRackauckas-Claude:add-expression-var-collector

Conversation

@ChrisRackauckas-Claude

Copy link
Copy Markdown
Member

Please ignore until reviewed by @ChrisRackauckas.

Summary

  • add ModelingToolkitBase.collect_expression_vars! as a public helper for collecting variables from arbitrary symbolic expressions
  • delegate classification to MTKBase's existing collect_var! logic so downstream packages do not need to duplicate parameter/unknown handling
  • document the helper and cover parameter, unknown, independent-variable, and default-expression collection behavior

Motivation

Catalyst needs to collect variables from expression leaves while preserving MTKBase's parameter/unknown classification behavior. Exposing this helper keeps that classification logic in MTKBase instead of copying it downstream.

Validation

  • julia +1.10 --project=lib/ModelingToolkitBase -e 'cd("lib/ModelingToolkitBase/test"); include("variable_utils.jl")' passed\n- julia +1.10 --project=lib/ModelingToolkitBase -e 'using ModelingToolkitBase; @assert isdefined(ModelingToolkitBase, :collect_expression_vars!); println(ModelingToolkitBase.collect_expression_vars!)' passed\n- Runic check passed on edited Julia files\n- git diff --check passed\n- GROUP=ModelingToolkitBase_InterfaceII julia +1.10 --project=lib/ModelingToolkitBase -e 'using Pkg; Pkg.test()' did not pass: InterfaceII reported 7153 passed, 2 errored, 4 broken, with both errors in Optimal Control + Constraints Tests BVP cases (Lotka-Volterra and Cost function compilation). The targeted Variable Utils Test section in that run passed with 139 tests.

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

Copy link
Copy Markdown
Member Author

CI triage note from local follow-up:

  • Runic / Runic Format Check fails on untouched files (lib/ModelingToolkitBase/src/systems/codegen.jl, index_cache.jl, src/initialization.jl, test/initialization_jacobian_analysis.jl). The edited Julia files passed Runic locally.
  • Spell Check with Typos reports existing Lamba/lambda annotations in untouched files.
  • docs build fails during dependency resolution before docs build (Optim/OptimizationOptimJL conflict).
  • Downgrade / Downgrade Tests - InterfaceI fails during dependency resolution on OrdinaryDiffEqCore.
  • The Catalyst checks relevant to the original LTS modeling issue were still pending at this point.

Local targeted validation remains:

  • julia +1.10 --project=lib/ModelingToolkitBase -e 'cd("lib/ModelingToolkitBase/test"); include("variable_utils.jl")' passed.\n- ModelingToolkitBase.collect_expression_vars! is defined and reachable in the MTKBase environment.

@AayushSabharwal

Copy link
Copy Markdown
Member

Why is this necessary when collect_vars! exists?

@ChrisRackauckas-Claude

Copy link
Copy Markdown
Member Author

Independent evidence for the question above: collect_vars! already exists, but its Julia 1.10 behavior is currently load-order dependent in the Catalyst failure that motivated this PR.

On a clean Catalyst ae55024a checkout, the repository GROUP=Modeling harness under Julia 1.10.11 reproduces DSL Options | 308 pass, 9 fail, 1 broken. Historical Catalyst CI gives a tight release boundary: the June 28 job passed with SymbolicUtils 4.37.1, and the June 29 job failed the 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 otherwise unchanged.

A reduced current-stack reproducer on Julia 1.10 behaves as follows:

  1. Constructing the programmatic ReactionSystem without loading Plots discovers all parameters [k, k_obs, k_ratio, eps_val].
  2. Loading Plots first makes that constructor discover only [k]; the next direct collect_vars! traversal of an observed RHS returns empty sets.
  3. Running the direct expression traversal first makes it succeed, but the following constructor discovers only the observed parameters and misses reaction parameter k.

That makes a direct expression walker useful as a diagnostic, but I agree that adding a second public collector risks two subtly different definitions of variable discovery. The stronger fix is to make the existing public collect_vars! path robust on Julia 1.10, or fold this implementation into collect_vars!, and add the Catalyst load-order case as a downstream regression. This appears to be the same compiler/invalidation class as #4211. As one negative probe, adding @noinline to the mutually recursive SymbolicT collect_vars!/collect_var! entry points did not fix the reduced reproducer.

The SymbolicUtils v4.37.1..v4.37.2 production window is the single ordering PR JuliaSymbolics/SymbolicUtils.jl#990 (first production commit f4f5ccc5c67302c5c647517cb97727b35c36da63, with comparator refactor 8bffce2e later in the same PR). The corresponding Catalyst audit is documented at SciML/Catalyst.jl#1503.

@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. With SymbolicUtils 4.37.1, the constructor returned [k, k_obs, k_ratio, eps_val], the next direct collect_vars! call returned unknowns [A(t), B(t)] and parameters [k_obs], and the assertion passed.

I changed only SymbolicUtils with:

Pkg.add(PackageSpec(name="SymbolicUtils", version=v"4.37.2"); preserve=Pkg.PRESERVE_ALL)

Pkg reported only 4.37.1 => 4.37.2. The identical program then returned constructor parameters [k], followed by empty unknown/parameter sets from direct collect_vars!, and the assertion failed. This directly confirms the 4.37.2 boundary and the existing collect_vars! load-order failure locally.

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.

3 participants