Skip to content

Fix lcons/ucons length in __extract_lcons_ucons for optimize path#473

Merged
ErikQQY merged 15 commits into
SciML:masterfrom
SebastianM-C:smc/fix-extract-lcons-length
May 1, 2026
Merged

Fix lcons/ucons length in __extract_lcons_ucons for optimize path#473
ErikQQY merged 15 commits into
SciML:masterfrom
SebastianM-C:smc/fix-extract-lcons-length

Conversation

@SebastianM-C
Copy link
Copy Markdown
Member

The per-dispatch reconstruction of the constraint length from (M, N, bcresid_prototype, f_prototype) was wrong for several discretizations:

  • MIRK with prob.lcons set padded to NM (decision-var length) instead of L_bc + M(N-1)
  • FIRK Expanded was off by a factor of stage+1
  • The ::Nothing f_prototype fallback used N*M everywhere

The authoritative value is length(resid_prototype), which every caller already has. Replace the 3 dispatches with a single one that takes constraint_length::Int directly, and update callers.

Incidentally fixes two multiple-shooting dispatches that referenced out-of-scope bcresid_prototype / f_prototype variables.

Adds:

  • Unit test in BoundaryValueDiffEqCore for both fallback and user-provided lcons/ucons paths
  • Integration regression test in BoundaryValueDiffEqMIRK for the StandardBVProblem + optimize path that previously threw BoundsError

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

Add any other context about the problem here.

The per-dispatch reconstruction of the constraint length from
(M, N, bcresid_prototype, f_prototype) was wrong for several
discretizations:
- MIRK with prob.lcons set padded to N*M (decision-var length)
  instead of L_bc + M*(N-1)
- FIRK Expanded was off by a factor of stage+1
- The ::Nothing f_prototype fallback used N*M everywhere

The authoritative value is length(resid_prototype), which every
caller already has. Replace the 3 dispatches with a single one that
takes constraint_length::Int directly, and update callers.

Incidentally fixes two multiple-shooting dispatches that referenced
out-of-scope bcresid_prototype / f_prototype variables.

Adds:
- Unit test in BoundaryValueDiffEqCore for both fallback and
  user-provided lcons/ucons paths
- Integration regression test in BoundaryValueDiffEqMIRK for the
  StandardBVProblem + optimize path that previously threw BoundsError

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 22, 2026

Benchmark Results

Click to check benchmark results
master 3e48f41... master / 3e48f41...
Simple Pendulum/IIP/BoundaryValueDiffEqMIRK.MIRK2() 0.583 ± 0.0035 s 0.584 ± 0.0012 s 0.999 ± 0.0063
Simple Pendulum/IIP/BoundaryValueDiffEqMIRK.MIRK3() 12.2 ± 0.84 ms 12.3 ± 0.79 ms 0.992 ± 0.094
Simple Pendulum/IIP/BoundaryValueDiffEqMIRK.MIRK4() 2.77 ± 0.45 ms 2.73 ± 0.44 ms 1.01 ± 0.23
Simple Pendulum/IIP/BoundaryValueDiffEqMIRK.MIRK5() 3.25 ± 0.57 ms 3.3 ± 0.52 ms 0.983 ± 0.23
Simple Pendulum/IIP/BoundaryValueDiffEqMIRK.MIRK6() 1.59 ± 0.33 ms 1.61 ± 0.3 ms 0.986 ± 0.28
Simple Pendulum/IIP/MultipleShooting(10, Tsit5; grid_coarsening = false) 1.81 ± 0.54 ms 1.84 ± 0.53 ms 0.987 ± 0.41
Simple Pendulum/IIP/MultipleShooting(10, Tsit5; grid_coarsening = true) 3.82 ± 1 ms 3.79 ± 1 ms 1.01 ± 0.37
Simple Pendulum/IIP/MultipleShooting(100, Tsit5; grid_coarsening = false) 0.0407 ± 0.0085 s 0.0408 ± 0.0073 s 0.998 ± 0.27
Simple Pendulum/IIP/MultipleShooting(100, Tsit5; grid_coarsening = true) 0.0666 ± 0.0084 s 0.0688 ± 0.018 s 0.968 ± 0.28
Simple Pendulum/IIP/Shooting(Tsit5()) 0.258 ± 0.077 ms 0.255 ± 0.075 ms 1.01 ± 0.42
Simple Pendulum/OOP/BoundaryValueDiffEqMIRK.MIRK2() 0.745 ± 0.017 s 0.745 ± 0.018 s 1 ± 0.034
Simple Pendulum/OOP/BoundaryValueDiffEqMIRK.MIRK3() 15.6 ± 6 ms 15.5 ± 6 ms 1.01 ± 0.55
Simple Pendulum/OOP/BoundaryValueDiffEqMIRK.MIRK4() 3.25 ± 0.31 ms 3.25 ± 0.24 ms 1 ± 0.12
Simple Pendulum/OOP/BoundaryValueDiffEqMIRK.MIRK5() 3.99 ± 0.48 ms 3.85 ± 0.2 ms 1.04 ± 0.14
Simple Pendulum/OOP/BoundaryValueDiffEqMIRK.MIRK6() 1.88 ± 0.43 ms 1.84 ± 0.42 ms 1.02 ± 0.33
Simple Pendulum/OOP/MultipleShooting(10, Tsit5; grid_coarsening = false) 3.71 ± 2.8 ms 3.81 ± 2.9 ms 0.973 ± 1.1
Simple Pendulum/OOP/MultipleShooting(10, Tsit5; grid_coarsening = true) 7.31 ± 5.6 ms 7.01 ± 5.4 ms 1.04 ± 1.1
Simple Pendulum/OOP/MultipleShooting(100, Tsit5; grid_coarsening = false) 0.0915 ± 0.0032 s 0.0907 ± 0.0071 s 1.01 ± 0.086
Simple Pendulum/OOP/MultipleShooting(100, Tsit5; grid_coarsening = true) 0.14 ± 0.0095 s 0.147 ± 0.014 s 0.954 ± 0.11
Simple Pendulum/OOP/Shooting(Tsit5()) 0.611 ± 0.097 ms 0.63 ± 0.08 ms 0.97 ± 0.2
time_to_load 7.53 ± 0.011 s 7.28 ± 0.12 s 1.03 ± 0.017
### Benchmark Plots A plot of the benchmark results has been uploaded as an artifact to the workflow run for this PR. Go to "Actions"->"Benchmark a pull request"->[the most recent run]->"Artifacts" (at the bottom).

@ErikQQY ErikQQY closed this Apr 30, 2026
@ErikQQY ErikQQY reopened this Apr 30, 2026
@ErikQQY ErikQQY merged commit 6e37525 into SciML:master May 1, 2026
22 of 24 checks passed
ChrisRackauckas-Claude pushed a commit to ChrisRackauckas-Claude/BoundaryValueDiffEq.jl that referenced this pull request May 1, 2026
PR SciML#473 already landed the equivalent fixes for MIRK and FIRK
(`copy(vec(cache.y₀))` in `__perform_*_iteration`, plus mesh-selection
and resize fixes), but missed the same `vec(::VectorOfArray)`
inference cliff in MIRKN's `__perform_mirkn_iteration`.

Under RecursiveArrayTools v4, `vec(::VectorOfArray)` returns a
`Base.ReshapedArray{T, 1, VectorOfArray{...}, …}` instead of a plain
`Vector`. NonlinearSolve's polyalg cannot infer `T, N, uType, R` of
the resulting `NonlinearSolution` when `u0` has that shape, so
`@inferred solve(::SecondOrderBVProblem, ::MIRKN, …)` would widen
the result type the same way it did for MIRK before SciML#473. Mirror
PR SciML#473's fix:

    nlprob = __construct_nlproblem(cache, vec(cache.y₀), copy(cache.y₀))
    →
    nlprob = __construct_nlproblem(cache, copy(vec(cache.y₀)), copy(cache.y₀))

Bumps `BoundaryValueDiffEqMIRKN` patch to 1.15.1.

Also adds a regression `@testitem` for issue SciML#484 (the original
adaptive-mesh-refinement `UndefRefError`) using the torus-geodesic
BVP from the issue.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
@SebastianM-C SebastianM-C deleted the smc/fix-extract-lcons-length branch May 1, 2026 23:38
ErikQQY pushed a commit to ChrisRackauckas-Claude/BoundaryValueDiffEq.jl that referenced this pull request May 3, 2026
- BoundaryValueDiffEqCore: 2.5.0 → 2.6.0
- BoundaryValueDiffEqMIRK: 1.16.0 → 1.17.0
- BoundaryValueDiffEqMIRKN: 1.15.1 → 1.16.0
- BoundaryValueDiffEqFIRK: 1.16.0 → 1.17.0
- BoundaryValueDiffEqShooting: 1.16.0 → 1.17.0

Bundles fixes accumulated since the last bump (b651399):
- Core: lcons/ucons length fix in __extract_lcons_ucons (SciML#473)
- MIRK: RAT mesh selection / global error control / view fix /
  initial guess fix / resize fix / type stability / Mooncake 0.5
- MIRKN: nlprob u0 inference fix (SciML#484/SciML#485)
- FIRK: Enzyme compat fix / RAT mesh selection / resize fix /
  Mooncake 0.5
- Shooting: initial guess fix

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.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