Skip to content

fix: don't leak the operating point into unbounded nonlinear bounds - #4902

Draft
ChrisRackauckas-Claude wants to merge 1 commit into
SciML:masterfrom
ChrisRackauckas-Claude:fix/nonlinear-bounds-symbolic-op-leak
Draft

fix: don't leak the operating point into unbounded nonlinear bounds#4902
ChrisRackauckas-Claude wants to merge 1 commit into
SciML:masterfrom
ChrisRackauckas-Claude:fix/nonlinear-bounds-symbolic-op-leak

Conversation

@ChrisRackauckas-Claude

Copy link
Copy Markdown
Member

Please ignore this PR until it has been reviewed by @ChrisRackauckas.

What changed and why

generate_nonlinear_bounds resolves symbolic bounds (e.g. bounds = (some_parameter, 1.0)) by merging the operating point into the bounds map. The map is seeded only for finite bounds:

for (var, b) in zip(dvs, lb)
    b === -Inf && continue                      # unbounded unknowns get no entry
    write_possibly_indexed_array!(lbmap, var, Symbolics.SConst(b), COMMON_NOTHING)
end
left_merge!(lbmap, op)                          # ...so `op` supplies them — as u0 values
lb = varmap_to_vars(lbmap, dvs; tofloat = false)

so the merge also hands every unbounded unknown its operating-point value. That pins the unknown to lb == ub == u0, and NonlinearSolveBase's bounds transform then computes logit((u - lb) / (ub - lb)) = logit(0/0) = NaN.

This fix restores the ±Inf sentinel after resolution for entries whose metadata bound was already ±Inf, leaving the resolution machinery (and the ability of a bound to reference another symbol) intact. The all-numeric branch never entered this path and is unchanged.

Found while investigating #4672. It is not that issue's reported failure — see the analysis in that thread — but it produces the NaN that issue's title describes, via a different route. It only fires when at least one bound is symbolic, which is why the numeric-bounds MWEs in #4672 don't hit it.

Failing before / passing after

Test added to the existing bounds testset in lib/ModelingToolkitBase/test/nonlinearsystem.jl.

Before the fix (test applied to unmodified source at 453c45c08e, source change stashed):

Symbolic bounds ... : Test Failed at testcase.jl:13
  Expression: sprob.lb[sdi] == -Inf && sprob.ub[sdi] == Inf
Symbolic bounds ... : Test Failed at testcase.jl:14
  Expression: SciMLBase.successful_retcode(ssol)
Symbolic bounds ... : Test Failed at testcase.jl:15
  Expression: !(any(isnan, ssol.u))

Test Summary:                       | Pass  Fail  Total   Time
Symbolic bounds ...                 |    1     3      4  26.1s
ERROR: Some tests did not pass: 1 passed, 3 failed, 0 errored, 0 broken.

Observed values on the unfixed code:

lb = [-1.0, 0.4]   ub = [1.0, 0.4]   u0 = [0.8, 0.4]     # 0.4 is the unbounded unknown's guess
solve  ->  retcode = Unstable,  u = [0.8, NaN]

After the fix:

Test Summary:                       | Pass  Total   Time
Symbolic bounds ...                 |    4      4  15.8s

Verification

GROUP=InterfaceII (the group that owns nonlinearsystem.jl), on this branch:

Test Summary:        | Pass  Total     Time
NonlinearSystem Test |  106    106  2m34.4s
...
4315.088507 seconds (3.78 G allocations: 359.776 GiB, 4.11% gc time, 85.43% compilation time)
     Testing ModelingToolkitBase tests passed

Runic: clean on both changed files. typos: clean.

Not verified / known gaps

  • GROUP=QA fails on this branch — but it fails identically on unmodified master. I ran QA on a clean 453c45c08e worktree to check: both give 10 passed, 1 failed, both failing the Aqua Piracy check at Aqua/src/piracies.jl:244, with a byte-identical method list (src/ModelingToolkitBase.jl:119,123,131,132,133 and src/problems/jumpproblem.jl:257,263). None of those files are touched here. Pre-existing master failure, reported separately; not introduced by this PR.
  • Docs build not run — this PR adds no public API and no rendered-docs entry (restore_infinite_bounds is an internal helper; its docstring is not registered in docs/).
  • Other test groups, downstream packages, and GPU paths not run.

Worth pushing back on

  • The helper restores the sentinel after resolution rather than seeding the map with ±Inf up front. Seeding would be tidier, but it would change behaviour for a bound that references another unknown: today such a bound resolves through that unknown's operating-point value, and seeding would make it resolve through that unknown's bound instead. I kept the existing semantics rather than silently changing them.

Environment: Julia 1.12.6, ModelingToolkitBase v1.61.0, NonlinearSolve v4.26.0, base commit 453c45c08e.

`generate_nonlinear_bounds` resolves symbolic bounds by merging the operating
point into the bounds map. The map is seeded only for finite bounds, so the
merge also hands every unbounded unknown its operating-point value. That pins
the unknown to `lb == ub == u0`, and the solver's bounds transform then
evaluates `logit((u - lb) / (ub - lb))` = `logit(0/0)` = `NaN`.

Restore the `±Inf` sentinel after resolution for entries whose metadata bound
was already `±Inf`. The all-numeric branch is unaffected.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
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