Skip to content

Don't analytically integrate variables modified by events#4826

Open
cmhyett wants to merge 1 commit into
SciML:masterfrom
cmhyett:fix-analytic-integ-check
Open

Don't analytically integrate variables modified by events#4826
cmhyett wants to merge 1 commit into
SciML:masterfrom
cmhyett:fix-analytic-integ-check

Conversation

@cmhyett

@cmhyett cmhyett commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

mtkcompile's zero-variable elimination moves variables with trivial dynamics (e.g. D(x) ~ 0) out of the unknowns, rewriting them as an observed polynomial in the independent variable with a missing-bound constant-of-integration parameter. This ignored discrete/continuous events: a variable a callback impulsively modifies is no longer an unknown, so a functional (ImperativeAffect) kick errors at ODEProblem construction ("... refers to missing variable(s) ... reduced away").

Fix it two ways:

  • Auto-detect (default): add variables_modified_by_events(sys) in ModelingToolkitBase and extend __eliminate_zero_variables! to protect event-modified variables using the same seam that already retains irreducible variables. Such a variable stays an ordinary numerically integrated unknown; unrelated trivial dynamics are still integrated.
  • Opt-out flag: mtkcompile(sys; analytic_integration=false) disables the transformation wholesale (threaded to eliminate_mm_zeros, composed via AND so the SDE force-off still wins).

Add a regression testset covering equational and imperative kicks, the flag, selective protection, and the event-free no-regression case.

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

Additional context

Single claude commit, needs deeper review - fix works but may not be most elegant.

`mtkcompile`'s zero-variable elimination moves variables with trivial
dynamics (e.g. `D(x) ~ 0`) out of the unknowns, rewriting them as an
observed polynomial in the independent variable with a `missing`-bound
constant-of-integration parameter. This ignored discrete/continuous
events: a variable a callback impulsively modifies is no longer an
unknown, so a functional (`ImperativeAffect`) kick errors at `ODEProblem`
construction ("... refers to missing variable(s) ... reduced away").

Fix it two ways:

- Auto-detect (default): add `variables_modified_by_events(sys)` in
  ModelingToolkitBase and extend `__eliminate_zero_variables!` to protect
  event-modified variables using the same seam that already retains
  `irreducible` variables. Such a variable stays an ordinary numerically
  integrated unknown; unrelated trivial dynamics are still integrated.
- Opt-out flag: `mtkcompile(sys; analytic_integration=false)` disables the
  transformation wholesale (threaded to `eliminate_mm_zeros`, composed via
  AND so the SDE force-off still wins).

Add a regression testset covering equational and imperative kicks, the
flag, selective protection, and the event-free no-regression case.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@cmhyett cmhyett mentioned this pull request Jul 24, 2026
5 tasks
@AayushSabharwal

AayushSabharwal commented Jul 24, 2026

Copy link
Copy Markdown
Member

Variables modified by standard symbolic events are fine. ImperativeAffect is the issue, since it doesn't run a custom reinitialization after the affect. Analytically integrated variables modified by ImperativeAffect can for now be marked as irreducible to avoid this issue. The flag to disable analytical integration can be useful, but it should specifically disable analytical integration and not the entire eliminate_zero_variables! pass. The analytic_integration flag added here is no different from the pre-existing eliminate_mm_zeros flag.

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