feat(madnlp): expose intermediate_callback + fixed_variable_treatment#98
Merged
Conversation
Add two pass-through fields to `MadNLPOptions` so users can install `MadNLP.AbstractUserCallback` subtypes for per-iteration hooks (e.g. live plotting of converging pulses, custom stopping criteria) and override MadNLP's handling of fixed bound variables. `fixed_variable_treatment = MadNLP.RelaxBound` is required when a callback needs the full primal vector — the default `MakeParameter` eliminates fixed variables from `solver.x` and breaks the `MadNLP.variable(solver.x)` → `traj.datavec` mapping that trajectory-reconstructing callbacks rely on. Both fields are forwarded by the existing generic `set_options!` loop and default to `nothing` (no behaviour change for existing users).
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Add an abstract callback type in the Solvers module so per-iteration hooks can be written once and reused across every solver backend. Subtypes implement `(cb)(primal::AbstractVector, iter::Integer) -> Bool`; each solver extension wraps an instance in a native-shape adapter at solve time. Wired up in MadNLP via `_MadNLPCallbackAdapter <: MadNLP.AbstractUserCallback`, which translates `solver.x → MadNLP.variable(solver.x)` and forwards `solver.cnt.k` as the iteration index. Raw `MadNLP.AbstractUserCallback` subtypes still pass through unwrapped for users wanting full IPM access. This unblocks downstream packages (e.g. Piccolo) from shipping plotting or analysis callbacks without taking a weak dependency on each solver.
Merged
2 tasks
Address review feedback on #98: - Filter mode to `UserCallbackRegular` in `_MadNLPCallbackAdapter`. MadNLP also invokes user callbacks during feasibility-restoration and robust phases, which would surface intermediate IPM state that's not meaningful to trajectory-level callbacks. Silently skipping those phases keeps `solver.cnt.k` monotonic from the callback's view. - Auto-couple `fixed_variable_treatment = MadNLP.RelaxBound` when an `AbstractIntermediateCallback` is installed without an explicit value. Emits an `@info` log so the override is visible. Raw MadNLP callbacks are untouched (they manage their own primal access). - Exhaustive type check on `intermediate_callback` — explicit `ArgumentError` for non-callback inputs instead of failing deep inside MadNLP's option parser. - `AbstractIntermediateCallback` docstring now documents the primal-aliasing contract (copy if retaining), iter monotonicity, and the MadNLP RelaxBound auto-coupling. - New testitems: - `auto-couples RelaxBound` — agnostic callback without explicit `fixed_variable_treatment` still receives the full primal. - `early termination via return false` — callback returning `false` after N iters halts the solve before `max_iter`. - `rejects invalid type` — bare `Function` raises `ArgumentError`.
Mirror MadNLP's own field signature (`fixed_variable_treatment::Type`) instead of leaving it as `::Any = nothing`. Catches bad inputs at the `MadNLPOptions` constructor (Strings, Symbols) rather than deep in MadNLP's option parser, while preserving `nothing` as the "defer to MadNLP's kkt_system-aware default" sentinel. The `set_options!` auto-couple branch is unchanged behaviourally (`=== nothing` still matches the default; `MadNLP.RelaxBound` is a `Type`, so the assignment is type-consistent). Explicit overrides (both `MadNLP.RelaxBound` and `MadNLP.MakeParameter`) still flow through correctly.
When an `AbstractIntermediateCallback` is installed without explicit `fixed_variable_treatment`, only override to `RelaxBound` if MadNLP's own conditional default would otherwise pick `MakeParameter` and break the callback. For `kkt_system <: SparseCondensedKKTSystem`, MadNLP's default is already `RelaxBound`, so leave its if-one-liner alone. Updated docstring on the field; added a testitem asserting that `kkt_system = SparseCondensedKKTSystem` bypasses the auto-couple (captured `@info` stream is empty for our message + primal length still matches the full NLP variable count).
The new "MadNLP auto-couple respects MadNLP's conditional default" testitem used `with_logger(SimpleLogger(...))` from the `Logging` stdlib, which isn't in DTO's test extras and failed to resolve in CI. Switch to `Test.collect_test_logs` — available via the testitem's auto-imported Test stdlib, no extra dep needed.
Merged
jack-champagne
added a commit
to harmoniqs/Piccolo.jl
that referenced
this pull request
May 27, 2026
#219) * feat(viz): LivePulsePlotCallback for live pulse plotting during solves Add a `LivePulsePlotCallback` that renders the current pulse via `plot_pulse(extract_pulse(qtraj, traj))` every IPM iteration, optionally saving a PNG per render. Useful for watching an optimization converge in real time and for generating animations of the trajectory's evolution. The callback subtypes `DirectTrajOpt.AbstractIntermediateCallback`, so it plugs into any DTO solver backend without taking weak dependencies on solver packages: cb = LivePulsePlotCallback(qtraj, qcp.prob.trajectory; save_dir = "out/") solve!(qcp; options = MadNLPOptions( intermediate_callback = cb, fixed_variable_treatment = MadNLP.RelaxBound, )) Stub lives at `src/visualizations/live_callbacks.jl` (always loaded); implementation lives in the existing `PiccoloMakieExt` extension and activates when a Makie backend is present. No new extension package required — works with `CairoMakie`, `GLMakie`, or `WGLMakie`. Depends on harmoniqs/DirectTrajOpt.jl#98 (`AbstractIntermediateCallback` abstraction). * docs: reflect DTO's auto-coupling of RelaxBound DTO #98's review pass added auto-coupling of `fixed_variable_treatment = MadNLP.RelaxBound` when an `AbstractIntermediateCallback` is installed without an explicit value. Drop the now-unneeded explicit setting from docstring example and testitem; update the length-mismatch warning to reflect that this state means the user has overridden the default. * review: tighten LivePulsePlotCallback + drop test-time MadNLP dep Address review feedback on #219: - Drop `import MadNLP` from the testitem. MadNLP isn't a Piccolo test dep (and adding it pulls in a heavy transitive tree for a single test). Replace the live-solve assertion with a direct-invocation test that synthesizes a primal vector — exercises the actual Piccolo behavior (stub → extension constructor, primal handling, plot/save) without coupling to the solver. End-to-end MadNLP coverage already lives in DTO #98's own tests. - Constructor validates `every >= 1` (an `every = 0` instance used to build fine and then DivideError on first invocation). - Move the `every` gate before `update!` to skip the trajectory copy on dropped iters. - `@warn` for primal-length mismatch now uses `maxlog = 1` so a user who explicitly overrode RelaxBound doesn't get N warnings. - Bump frame filename padding from 3 to 5 digits — keeps lexical sort correct up to `iter_99999.png`. - Three new testitems cover: type contract + direct invocation + frame distinctness; `every > 1` skipping (4 frames over 11 invocations with `every = 3`); `every <= 0` rejection. * chore: autoformat
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds the plumbing needed to install solver-agnostic per-iteration callbacks during a
solve!, with MadNLP wired up. Two layers:Layer 1 — MadNLP-specific pass-throughs (
MadNLPOptions)intermediate_callback::Any = nothing— install any per-iter callback. Accepts either a rawMadNLP.AbstractUserCallback(passed through unwrapped) or aDirectTrajOpt.AbstractIntermediateCallback(wrapped in an internal adapter). Returnfalseto terminate the solve (yieldsUSER_REQUESTED_STOP).fixed_variable_treatment::Any = nothing— override MadNLP's handling of variables withlb == ub. Default behaviour unchanged; passMadNLP.RelaxBoundwhen a callback needs the full primal vector.Layer 2 — solver-agnostic abstraction (
DirectTrajOpt.AbstractIntermediateCallback)A new abstract type lives in the
Solverssubmodule. Subtypes implement the uniform signatureEach solver extension owns the adapter that translates this into the native callback shape. MadNLP's adapter (
_MadNLPCallbackAdapter <: MadNLP.AbstractUserCallback) wraps(solver, mode) → (variable(solver.x), solver.cnt.k). Future Ipopt wiring will add a parallel adapter insideIpoptSolverExt.This means downstream packages (Piccolo, etc.) can ship per-iter callbacks — live pulse plotting, custom stopping criteria, analysis hooks — by subtyping
AbstractIntermediateCallbackonce, without taking a weak dep on every solver backend.Why both fields
A callback that wants to reconstruct a
NamedTrajectoryfrom the primal vector (the use case that motivated this — live pulse plotting during optimization) cannot work with MadNLP's defaultMakeParameter, which eliminates fixed boundary variables from the IPM's working primal vector. The two fields ship together so that pattern works out-of-the-box.Behaviour change
None for existing users. Both fields default to
nothingand are skipped by the existing genericset_options!loop.Test plan
MadNLPOptions constructiontestitem extended to assert both defaults arenothingMadNLP intermediate_callback (raw MadNLP callback) fires per iter— verifies aMadNLP.AbstractUserCallbacksubtype runs on every IPM iterationMadNLP intermediate_callback (AbstractIntermediateCallback) fires per iter— verifies the solver-agnostic adapter path: callback is wrapped, receives full primal vector matchinglength(traj.datavec) + traj.global_dim(withRelaxBound)SmoothPulseProblemX-gate solve with aLivePulsePlotCallback <: AbstractIntermediateCallbacksaving a PNG per iter via Piccolo'splot_pulse. Wiring is solid; the rendered frames track the converging pulse correctly.Follow-ups
IpoptSolverExtso the sameAbstractIntermediateCallbackinstance works with both solvers.LivePulsePlotCallback <: AbstractIntermediateCallbackfromPiccoloMakieExt(no new triple-trigger extension needed thanks to this abstraction).