Skip to content

test: restore working MTKBase BVP assertions - #4843

Merged
ChrisRackauckas merged 1 commit into
SciML:masterfrom
ChrisRackauckas-Claude:agent/fix-mtkbase-bvp-broken-tests
Jul 30, 2026
Merged

test: restore working MTKBase BVP assertions#4843
ChrisRackauckas merged 1 commit into
SciML:masterfrom
ChrisRackauckas-Claude:agent/fix-mtkbase-bvp-broken-tests

Conversation

@ChrisRackauckas-Claude

Copy link
Copy Markdown
Member

Ignore this PR until it has been reviewed by @ChrisRackauckas.

Summary

  • restore the four ModelingToolkitBase BVP solve paths that currently succeed
  • assert their solution values and successful return codes instead of passing solution objects to Test.@test_broken
  • leave the genuinely broken parameter-estimation cases marked broken

Cause

Commit f71cc7d wrapped four successful BVP solves in Test.@test_broken. A successful solve returns a solution object, not a Boolean, so Test reports each case as an error rather than a broken test.

This is the same failure in BoundaryValueDiffEq clean master run https://github.com/SciML/BoundaryValueDiffEq.jl/actions/runs/30342212061 and PR #541. The PR #541 shooting changes are unrelated.

Verification

Using Julia 1.11 and the official ModelingToolkitBase InterfaceII group with local BoundaryValueDiffEqCore, BoundaryValueDiffEqMIRK, and BoundaryValueDiffEqAscher:

  • clean ModelingToolkit master at a35a814: BVP tests report 14 passed, 4 errored, 6 broken; the group fails
  • this branch: BVP tests report 22 passed, 6 broken; the complete InterfaceII group passes in 3707.7 seconds

A bisect from 3c2e4a7 to a35a814 identified f71cc7d as the first bad commit.

The changed file passes Runic and git diff --check. Whole-repository Runic currently fails on unchanged master in src/systems/clock_inference.jl; that independent FormatCheck regression is intentionally not mixed into this focused PR.

@ChrisRackauckas-Claude

Copy link
Copy Markdown
Member Author

Investigation scratchpad:

  1. Read the failing BoundaryValueDiffEq master and PR #541 InterfaceII logs and matched all four errors to lib/ModelingToolkitBase/test/bvproblem.jl.
  2. Reproduced the official downstream workflow locally against clean ModelingToolkit SHA a35a814 and BoundaryValueDiffEq SHA 219b502: 14 pass, 4 error, 6 broken in the BVP testset.
  3. Ran git bisect between known-good 3c2e4a7 and failing a35a814; first bad commit was f71cc7d.
  4. Restored the existing value and return-code assertions for the four successful solves.
  5. Ran the complete ModelingToolkitBase InterfaceII group on this branch with the same local BoundaryValueDiffEq sublibraries: 22 pass, 6 broken in the BVP testset and 3707.7 seconds overall, exit 0.
  6. Ran Runic on the changed file and git diff --check successfully. Whole-tree Runic also reproduces the independent clock_inference.jl failure on clean current master.

@ChrisRackauckas-Claude

Copy link
Copy Markdown
Member Author

CI classification: the non-Runic reds are all present on the exact base commit 529c98930e before this PR. Master run 30363096231 has the same Catalyst, ModelingToolkitStandardLibrary, and SBMLToolkit failures, and master sublibrary run 30363097045 has the same ModelingToolkitBase QA failure (as well as the pre-existing InterfaceII failures this PR fixes). The failures are unrelated to this PR’s single @test_broken cleanup.

The Runic failure is also reproduced on clean master and is confined to src/systems/clock_inference.jl; a separate one-line explicit-return fix is under whole-repository Runic plus full InterfaceI validation. I am keeping that independent change out of this focused PR.

@ChrisRackauckas-Claude

Copy link
Copy Markdown
Member Author

More specifically, the Catalyst job’s seven errors are all “Unexpected Pass” from stale @test_broken markers in conservation-law tests; focused Catalyst draft #1512 already activates exactly those seven assertions and has direct/full-group verification. That downstream fix is also independent of this ModelingToolkitBase test cleanup.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
@ChrisRackauckas-Claude
ChrisRackauckas-Claude force-pushed the agent/fix-mtkbase-bvp-broken-tests branch from 7cbbc00 to cbd487c Compare July 29, 2026 12:35
@ChrisRackauckas-Claude

Copy link
Copy Markdown
Member Author

Independent clean-main audit after rebasing onto current master:

  • Julia 1.12.6, current compatible BoundaryValueDiffEqMIRK 1.17.3 and BoundaryValueDiffEqAscher 1.15.2.
  • Clean current main standalone Lotka–Volterra path: 0 pass, 2 errors. Both @test_broken solve(...) expressions returned valid numerical solution objects and Test reported “Expression evaluated to non-Boolean.”
  • Executed adjacent bisect endpoints with the same dependency state: 05f590ee3 passed the original value assertions 4/4 in 5m08.2s; f71cc7d5 errored 2/2 in 5m07.7s with the same numerical solutions. git bisect therefore identifies f71cc7d5 as the first bad commit. This is a test regression, not a later solver/dependency regression.
  • Rebased branch standalone bvproblem.jl: 22 passed, 6 pre-existing broken, exit 0. Repeated after the final rebase onto cdb7ad542; same 22/6 result, exit 0.
  • Official GROUP=InterfaceII julia +1.12 --project=. -e "using Pkg; Pkg.test()": final BVP safetestset 22 passed / 6 broken; entire ModelingToolkitBase group passed, exit 0, 3940.14s.
  • Runic 1.7.0 passes the changed file and git diff --check passes. Whole-tree Runic still reproduces the separate clean-master src/systems/clock_inference.jl missing-explicit-return failure; that remains outside this focused PR.

The lease-protected rebase push updated the branch from 7cbbc001f to tested commit cbd487c74 without overwriting any remote-only commit.

@ChrisRackauckas
ChrisRackauckas marked this pull request as ready for review July 30, 2026 00:38
@ChrisRackauckas
ChrisRackauckas merged commit 91f17e3 into SciML:master Jul 30, 2026
68 of 92 checks passed
SebastianM-C added a commit to SebastianM-C/ModelingToolkit.jl that referenced this pull request Aug 6, 2026
The "Parameter estimation" testset in bvproblem.jl accounts for essentially the
entire memory peak of the file, and is the cause of the MTKBase/InterfaceII CI
runner OOM. `dt = 1e-3` over `tspan = (0, 1)` builds a 1001-point collocation
mesh — 4004 Ipopt variables and 26010 Lagrangian-Hessian nonzeros — differentiated
by a fallback sparse `SecondOrder` AD.

That mesh dates from when the test was added (2026-02-26), while the MIRK
interpolation bug fixed by BoundaryValueDiffEq.jl SciML#452 was still present; a fine
mesh limits interpolation error at the 51 `EvalAt` cost points. SciML#452 merged
2026-03-30 and the `dt` was never revisited. It now buys nothing:

    dt = 1e-3    7.20 GiB   3m34   α = 1.8000275184   γ = 6.4998637712
    dt = 1e-2    2.15 GiB   1m02   α = 1.8000278445   γ = 6.4998629391

Same answer to six significant figures for 70% less memory and 71% less time.

Also remove the `if @isdefined(ModelingToolkit)` gate. It currently makes CI
error rather than skip: the `else` branch is `@test_broken solve(...)`, and the
solve now succeeds and returns an ODESolution, which `@test_broken` reports as
"Expression evaluated to non-Boolean". This is the same failure mode SciML#4843 fixed,
at a fifth site it missed. The gated-off path is not broken — run with only
ModelingToolkitBase loaded, both assertions pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
SebastianM-C added a commit to SebastianM-C/ModelingToolkit.jl that referenced this pull request Aug 6, 2026
The "Parameter estimation" testset in bvproblem.jl accounts for essentially the
entire memory peak of the file, and is the cause of the MTKBase/InterfaceII CI
runner OOM. `dt = 1e-3` over `tspan = (0, 1)` builds a 1001-point collocation
mesh — 4004 Ipopt variables and 26010 Lagrangian-Hessian nonzeros — differentiated
by a fallback sparse `SecondOrder` AD.

That mesh dates from when the test was added (2026-02-26), while the MIRK
interpolation bug fixed by BoundaryValueDiffEq.jl SciML#452 was still present; a fine
mesh limits interpolation error at the 51 `EvalAt` cost points. SciML#452 merged
2026-03-30 and the `dt` was never revisited. It now buys nothing:

    dt = 1e-3    7.20 GiB   3m34   α = 1.8000275184   γ = 6.4998637712
    dt = 1e-2    2.15 GiB   1m02   α = 1.8000278445   γ = 6.4998629391

Same answer to six significant figures for 70% less memory and 71% less time.

Also remove the `if @isdefined(ModelingToolkit)` gate. It currently makes CI
error rather than skip: the `else` branch is `@test_broken solve(...)`, and the
solve now succeeds and returns an ODESolution, which `@test_broken` reports as
"Expression evaluated to non-Boolean". This is the same failure mode SciML#4843 fixed,
at a fifth site it missed. The gated-off path is not broken — run with only
ModelingToolkitBase loaded, both assertions pass.

Co-Authored-By: Claude Opus 5 (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