Skip to content

Add shape probe: measure "1-D = scalar" boundary compliance - #614

Open
ocots wants to merge 13 commits into
mainfrom
shape/probe-scalar-audit-613
Open

Add shape probe: measure "1-D = scalar" boundary compliance#614
ocots wants to merge 13 commits into
mainfrom
shape/probe-scalar-audit-613

Conversation

@ocots

@ocots ocots commented Jul 27, 2026

Copy link
Copy Markdown
Member

Summary

  • Adds probe/shape/ — a diagnostic (not a test suite) that drives a type-recording OCP through CTDirect's ADNLP transcription to measure, without modifying any source, what shapes reach the user's dynamics/lagrange/mayer/path/boundary functions today, and the cost of each candidate fix (getter-level vs. call-boundary coercion). Mirrors CTFlows.jl/probe/cpu.
  • Ground-truth for the "1-D = scalar" work (Handbook philosophy/dimension-and-shape.md), part of roadmap #610 §7 / discussion #609 §7.

Closes #613

Test plan

  • julia --project=probe/shape probe/shape/probe_shape.jl runs clean and prints the capability matrices

A diagnostic (not a test suite) that drives a type-recording OCP through
CTDirect's ADNLP transcription to measure, without modifying any source,
what shapes reach the user's dynamics/lagrange/mayer/path/boundary
functions today, and what each candidate fix (getter-level vs
call-boundary coercion) would cost. Mirrors CTFlows.jl/probe/cpu.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@github-actions
github-actions Bot requested a review from PierreMartinon July 27, 2026 19:58
Base automatically changed from refactor/docp-dispatch to main July 27, 2026 20:00
@github-actions

github-actions Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Breakage test results
Date: 2026-07-27 20:02:25

Name Latest Stable
OptimalControl compat: v1.0.12 compat: v1.0.12

@ocots

ocots commented Jul 27, 2026

Copy link
Copy Markdown
Member Author

Why Option B (call-boundary coercion), not Option A (getter-level coercion)

Two places could apply the only/identity coercion: inside the NLP-vector getters (get_OCP_state_at_time_step, get_OCP_control_at_time_step, get_OCP_variable), or immediately before each call into a CTModels user function (dynamics, lagrange, mayer, path_constraints_nl, boundary_constraints_nl).

Option A looks cheaper at first glance — three functions instead of ~16 call sites — but the probe shows it's incomplete by construction, not just costlier:

  • The getters are not solely a user-function boundary. They also feed get_time_grid (→ CTModels.final_time(ocp, v) — works either way, CTModels has a ctNumber method), build_OCP_solution's v[:] = <result> (breaks on a scalar: ArgumentError), and trapeze's x_next += h*(w_i + w_{i+1}) (breaks on a scalar: MethodError). Those two are fixable, but they're evidence the getters carry more consumers than the "shape it for the user" job.
  • The decisive one: the IRK / IRK-stagewise stage state never passes through a getter at all. work_xij is a work buffer built by in-place accumulation (@. work_xij = work_xij + h * a_jl * k_il) and handed straight to dynamics(...). Coercing the getters can't touch it — it would need a second, separate coercion mechanism at the IRK dynamics/Lagrange call sites, with the IRK path silently non-compliant if anyone forgot to add it.

So Option A reduces to Option B + a second mechanism + two internal-algebra rewrites. Option B — precomputing cx/cu/cv once from the OCP's declared dimensions and applying them right before each user-function call, buffers left untouched — is complete in one pass, matches "the shape contract is set by whoever calls the function" (Handbook), and is exactly the shape CTFlows already ships (_dim_coerce/_coerce_state in CTFlows.Systems/CTFlows.Flows). It's also the form that survives the §3 recipe rework: the coercion becomes a shape ingredient carried by the recipe, applied at the emission points — the getters Option A would touch are precisely what §3 replaces with layout index-range accessors.

Explicitly out of scope here (and why)

  • GPU-safe only (CTFlows' _safe_only). CTDirect's ADNLP transcription is CPU-only today, so plain Base.only is correct and avoids pulling GPUArraysCore into CTDirect for no reason. If/when a GPU-backed ADNLP path exists, swapping in a _safe_only-equivalent is a one-function change (same call sites, same signature).
  • ExaModels / Exa backend. That transcription still lives in CTParser, not CTDirect (roadmap [Roadmap] Consolidate CTDirect following the Handbook philosophy #610 §5 — "own the ExaModels transcription"). Exa is functional-style (no in-place buffer), so its shape boundary needs separate treatment once it's repatriated here; bolting a CPU-ADNLP-shaped fix onto it now would likely need redoing.
  • ode/variable.jl (direct-shooting variable-step ODE). Not even included in CTDirect.jl today, and has an error("type ?") mid-file — it's half-finished independently of this work. Align it under §10 (multiple shooting rework) instead of carrying dead code through this change.
  • Promoting the coercion primitive to CTBase.Core. CTBase.Core.make_coerce already exists but dispatches on the runtime value (make_coerce(::Number) = only), which is the opposite of what the Handbook asks for here (dimension-declared, not value-typed). Since CTFlows and CTDirect would need different onlys (GPU-safe vs. plain) if unified today, sharing a primitive is a cross-package decision better made once §5 forces the GPU question, not before.

Detailed audit (measured capability matrices, full call-site inventory, test sketches) to follow in a separate comment/report on this PR.

ocots and others added 2 commits July 27, 2026 22:26
Follow the control-toolbox Handbook workflow philosophy (WORKFLOWS.md §2)
and the pattern already shipped in CTFlows.jl: expensive workflows only
run on push/tag to main, and are opt-in per PR via a 'run …' label. This
avoids burning CI/doc-build/breakage minutes on every push to a draft PR
such as this one.

- CI.yml: split into test-cpu-github (ubuntu/macos/windows, label
  'run ci cpu') and test-gpu-kkt (self-hosted kkt runner, label
  'run ci gpu'), mirroring CTFlows.jl's split-CI structure. GPU.yml is
  kept as-is (it runs the dedicated GPU-specific test suite via
  test_args=GPU, a different job than the plain suite on the kkt runner).
- Documentation.yml: gate behind 'run documentation'.
- Breakage.yml: add the missing label gate ('run breakage') — it
  previously ran unconditionally on every PR push.
- All three: drop 'opened' from the pull_request 'types' list. A label
  applied at PR creation emits both 'opened' and 'labeled'; keeping
  'opened' in a label-gated caller fires the job twice on the same
  commit (see WORKFLOWS.md §2 for the measured duplicate-run evidence).

Created the 'run ci cpu', 'run ci gpu', 'run documentation', and
'run breakage' repo labels (none of the four existed before).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
test-gpu-kkt now passes test_args: 'GPU' (new input on CTActions'
reusable ci.yml, control-toolbox/CTActions#65) to run CTDirect's
GPU-specific test suite (test/test_gpu.jl) directly, instead of the
plain suite. This was the only reason GPU.yml existed as a separate
hand-rolled workflow, so it is removed along with the now-orphaned
'run GPU' label ('run ci gpu' on test-gpu-kkt is the single GPU
trigger from here on).

Dropped with GPU.yml: the weekly cron schedule and continue-on-error.
Discussed with the user; simplification (one GPU path, PR-gated only)
was the preferred tradeoff over keeping both.

Depends on control-toolbox/CTActions#65 landing on @main before
test-gpu-kkt can resolve the test_args input.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@ocots

ocots commented Jul 27, 2026

Copy link
Copy Markdown
Member Author

Scope note: existing test problems are not migrated to scalar style here.

The implementation adds one new test problem (test/problems/scalar_style_integrator.jl) written in the idiomatic scalar style (r[1] = -x + u, no [1] on the inputs), used purely as a non-regression pin: it fails today (MethodError) and passes once the call-boundary coercion lands.

All the existing test problems (both @def-based and functional-API) are left untouched. This is intentional and not risky: every 1-D user function already in the repo is [1]-indexed (u[1], v[1], etc.), so it evaluates identically whether it's handed a Number or a length-1 vector — that's exactly the "safe common denominator" the Handbook's migration note describes. There is nothing to fix for correctness.

That said, rewriting them to drop the now-unnecessary [1] indexing would be more consistent with the "1-D = scalar" convention once this lands, and would make the existing suite a better advertisement of the idiom for anyone reading it. I'm deliberately leaving that as a follow-up cleanup PR rather than bundling it here, to keep this diff mechanical and easy to review.

Add _dim_coerce/DOCPshape, precomputed once per DOCP and applied immediately
before every call into a user OCP function (dynamics, lagrange, mayer, path
and boundary constraints), across all 7 schemes. Matches CTFlows' existing
convention: 1-D state/control/variable now reach the user as a scalar, driven
by the declared dimension, never by runtime type; the in-place derivative
buffer stays a vector.

Add a scalar-style non-regression problem and a type-recording contract test
suite (all schemes, 1-D/n-D/mixed-dim/zero-control cases) confirming the
contract holds with no behaviour change to existing (all [1]-indexed)
problems. Full suite: 535/535.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@ocots ocots added the run ci github-runner Trigger CI on GitHub runners label Jul 27, 2026
ocots and others added 5 commits July 27, 2026 23:56
Points test-cpu-github at CTActions@fix/windows-ct-registry and limits
the matrix to windows-latest only, to check whether removing the
runner.os != 'Windows' guard lets add-julia-registry@v2 work on this
runner. To be reverted once the result is known.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@ocots ocots removed the run ci github-runner Trigger CI on GitHub runners label Jul 28, 2026
CTActions#66 (OpenSSH client swap + cache-before-registry ordering) is
merged to main, so windows-latest works with use_ct_registry again.
Points test-cpu-github back at @main and restores the full
ubuntu/macos/windows matrix, ending the temporary experiment against
the fix/windows-ct-registry branch.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@ocots ocots added the run ci github-runner Trigger CI on GitHub runners label Jul 28, 2026
@ocots
ocots marked this pull request as ready for review July 28, 2026 06:33
@ocots ocots removed the run ci github-runner Trigger CI on GitHub runners label Jul 28, 2026
@ocots ocots added the run ci kkt-runner Trigger CI on kkt runner label Jul 28, 2026
@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.72131% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.25%. Comparing base (f8b7732) to head (30faf01).

Files with missing lines Patch % Lines
src/ode/midpoint.jl 80.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #614      +/-   ##
==========================================
+ Coverage   70.64%   71.25%   +0.61%     
==========================================
  Files          14       14              
  Lines        1366     1395      +29     
==========================================
+ Hits          965      994      +29     
  Misses        401      401              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…ates

MadNLPGPU 0.10 moved CUDA/CUDSS support to a package extension that only
loads when CUDSS is imported alongside CUDA, unlike 0.8 where CUDSS was a
hard dependency. test_gpu.jl already defaulted to linear_solver=CUDSSSolver
but never imported CUDSS, causing the kkt GPU job to fail.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@ocots ocots removed the run ci kkt-runner Trigger CI on kkt runner label Jul 30, 2026
@ocots ocots closed this Jul 30, 2026
@ocots ocots added run ci github-runner Trigger CI on GitHub runners run ci kkt-runner Trigger CI on kkt runner labels Jul 30, 2026
@ocots ocots reopened this Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run ci github-runner Trigger CI on GitHub runners run ci kkt-runner Trigger CI on kkt runner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1-D = scalar: pass scalars for 1-D state/control/variable to user OCP functions (§7)

2 participants