diff --git a/BREAKING.md b/BREAKING.md index 188de5b5..2f9415be 100644 --- a/BREAKING.md +++ b/BREAKING.md @@ -23,9 +23,9 @@ Without it, `Flow` fails with a bare `MethodError`. This is by design: it keeps | v2.0 | v2.1.0-beta | Notes | | --- | --- | --- | -| `Lie(X, f)` | `ad(X, f)` | renamed | -| `X ⋅ f` | `ad(X, f)` | **removed**, no alias | -| `HamiltonianLift` | `CTLie.LiftedHamiltonianFunction` | renamed **and** re-parented | +| `Lie(X, f)` | `ad(X, f)` | renamed; `Lie(...)` now throws `PreconditionError` | +| `X ⋅ f` | `ad(X, f)` | removed; `X ⋅ f` now throws `PreconditionError` | +| `HamiltonianLift` | `CTLie.LiftedHamiltonianFunction` | renamed; `HamiltonianLift(...)` now throws `PreconditionError` | `LiftedHamiltonianFunction` is `<: Function`, no longer `<: AbstractHamiltonian`. Any `isa` or `<:` test against the old hierarchy is now wrong: @@ -50,9 +50,24 @@ f(t0, x0, p0, tf; variable=λ) f(t0, x0, p0, tf; variable=λ, variable_costate=true) ``` -1. **There is no positional slot for the variable any more**, and `variable=` is **mandatory** on a `NonFixed` problem. Omitting it raises a `PreconditionError` whose suggestion is literally *"Pass `variable=v` when calling the flow"* — it does not silently default. -2. **`augment=true` → `variable_costate=true`.** It integrates the augmented adjoint `ṗᵥ = -∂H/∂v` and returns `(xf, pf, pvf)` instead of `(xf, pf)`. +1. **There is no positional slot for the variable any more**, and `variable=` is **mandatory** on a `NonFixed` problem. The old positional spellings `f(t0, x0, p0, tf, λ)` and `f(t0, x0, tf, λ)` raise a `PreconditionError` suggesting `variable=λ`; omitting it raises a `PreconditionError` whose suggestion is literally *"Pass `variable=v` when calling the flow"* — it does not silently default. +2. **`augment=true` → `variable_costate=true`.** It integrates the augmented adjoint `ṗᵥ = -∂H/∂v` and returns `(xf, pf, pvf)` instead of `(xf, pf)`. The old spelling is not shimmed — it fails with a bare `MethodError` today, not a `PreconditionError` — because a shim here would mean overwriting CTFlows' own method; filed as [CTFlows#402](https://github.com/control-toolbox/CTFlows.jl/issues/402) instead. 3. **New `unsafe=false`.** With `unsafe=true` the ODE retcode is not checked and failures do not throw — useful inside a shooting loop, where an intermediate failure should surface through the residual. +4. **Integrator options can no longer be overridden per call.** In v2.0, keywords like `saveat=`, `abstol=`, `reltol=`, `alg=` were forwarded straight through to OrdinaryDiffEq.jl at *call* time: + + ```julia + # before — worked at call time + f(t0, x0, p0, tf; abstol=1e-8) + f((t0, tf), x0, p0; saveat=range(t0, tf, 100)) + ``` + + The call signature now only accepts `variable`, `unsafe` and `variable_costate`; anything else is a bare `MethodError`, not a `PreconditionError` — this spelling is **not shimmed** (it would mean overwriting CTFlows' own call method, the same reason `Flow(ocp, u, g, μ)` above and `augment=` are not shimmed either). Pass integrator options at **construction** time instead, where they still work exactly as before: + + ```julia + # after — set once, at construction + f = Flow(ocp, (x, p) -> p[2]; abstol=1e-8) + f(t0, x0, p0, tf) + ``` ## Constrained flows: keywords replace positional arguments @@ -64,6 +79,8 @@ fb = Flow(ocp, u, g, μ) # 3 positional fb = Flow(ocp, u; constraint=g, multiplier=μ) # paired keywords ``` +The old positional form already raises a `PreconditionError` today, but with a misleading suggestion — fix filed as [CTFlows#401](https://github.com/control-toolbox/CTFlows.jl/issues/401). + The two are a pair: one without the other is an `IncorrectArgument`. `constraint` now accepts three spellings, which is a capability gain rather than a rename — a plain `Function`, a `Data.PathConstraint`, **or a `Symbol` naming a `:path` constraint already declared in the OCP**: @@ -90,8 +107,8 @@ The old spelling on `@Lie` raises an `IncorrectArgument` at macro-expansion time | Name | Why | | --- | --- | -| `time` | It is `Base.time`, extended but not exported by `CTModels.Components`. Get it from `Base`. | -| `success` | `CTModels.Solutions` exports the name but defines no method for it, so `success(sol)` was always a `MethodError`. **Use `successful(sol)`**, which is the real accessor and is unchanged. | +| `time` | It is `Base.time`. `time(ocp)` and `time(sol)` now throw `PreconditionError`; use `times(ocp)` or `time_grid(sol)`. | +| `success` | `Base.success` is the name. `success(sol)` now throws `PreconditionError`; use `successful(sol)`. | ## Newly re-exported diff --git a/_typos.toml b/_typos.toml index 5d65cd54..3e0d30e0 100644 --- a/_typos.toml +++ b/_typos.toml @@ -8,7 +8,9 @@ extend-ignore-re = [ "exemples", "choses", "certains", - "remplace" + "remplace", + "maintenant", + "construit" ] [files] diff --git a/docs/reports/10-migration.md b/docs/reports/10-migration.md index a7758e6b..e916ae3a 100644 --- a/docs/reports/10-migration.md +++ b/docs/reports/10-migration.md @@ -59,8 +59,9 @@ already in the repo: `src/helpers/describe.jl:51-55` pirates | `Flow(f::Function)` | yes — piracy on `CTFlows.Flows.Flow` | no | **do it** | | flow call `f(t0,x0,p0,tf,λ)` | yes — one method on the `AbstractHamiltonianFlow` alias | no | **do it** — best value/cost of the set | | flow call `f(t0,x0,tf,λ)` (state flow) | yes — same pattern on `AbstractStateFlow` | no | **do it**, for symmetry | -| `Flow(ocp, u, g, μ)` | only as a 4-arity specialisation; the exact signature would **overwrite** upstream | **yes** — `PreconditionError` at `CTFlows/…/src/Flows/building.jl:1019` | **skip**; open a CTFlows issue (its `suggestion` string is wrong for this case) | -| flow call `augment=true` | **no** — Julia cannot dispatch on a keyword name | no | **skip**; CTFlows issue | +| `Flow(ocp, u, g, μ)` | only as a 4-arity specialisation; the exact signature would **overwrite** upstream | **yes** — `PreconditionError` at `CTFlows/…/src/Flows/building.jl:1019` | **skip**; filed [CTFlows#401](https://github.com/control-toolbox/CTFlows.jl/issues/401) (its `suggestion` string is wrong for this case) | +| flow call `augment=true` | **no** — same positional signature as the still-valid call, so a shim would overwrite CTFlows' own `OptimalControlFlow` method; confirmed this breaks precompilation (`ERROR: Method overwriting is not permitted during Module precompilation`) | no | **skip**; filed [CTFlows#402](https://github.com/control-toolbox/CTFlows.jl/issues/402) | +| flow call `f(...; saveat=, abstol=, reltol=, alg=, ...)` (per-call integrator option override) | **no** — same reason as `Flow(ocp, u, g, μ)`: the call signature is closed (`variable`/`unsafe`/`variable_costate` only), so a shim would overwrite CTFlows' own call method | no — bare `MethodError`, not caught anywhere | **skip**; document in `BREAKING.md` (§"Flow call convention" point 4). Construction-time options (`Flow(ocp, u; abstol=...)`) are unaffected and still work. Not filed upstream: this looks like a deliberate CTFlows design choice (options are baked into the flow's type at construction), not a bug — unlike the two rows above. | | `@Lie … autonomous=false` | n/a | **yes** — `IncorrectArgument` at `CTLie/src/lie_macro.jl:381` | **skip** | | `autonomous=` / `variable=` / `inplace=` on the `Data` constructors | **no** — 14 entry points, and the workaround duplicates CTBase's trait detection | no | **skip**; optional CTBase issue | @@ -189,16 +190,16 @@ Test.@test getfield(OptimalControl, :⋅) === LinearAlgebra.dot ## Acceptance criteria (PR 3) -- [ ] `src/deprecated.jl` exists, is included after `imports/`, and has the piracy banner +- [x] `src/deprecated.jl` exists, is included after `imports/`, and has the piracy banner plus the "not shimmed, and why" list. -- [ ] `Lie`, `⋅`, `HamiltonianLift` are exported; `success`, `time`, `Flow` are not +- [x] `Lie`, `⋅`, `HamiltonianLift` are exported; `success`, `time`, `Flow` are not re-exported by this file. -- [ ] `using OptimalControl, LinearAlgebra` produces **no** export-conflict warning. -- [ ] Every shim's message names its replacement, verified by an `occursin` assertion. -- [ ] The three `test_ctlie.jl` assertions are rewritten; the full suite is green via +- [x] `using OptimalControl, LinearAlgebra` produces **no** export-conflict warning. +- [x] Every shim's message names its replacement, verified by an `occursin` assertion. +- [x] The three `test_ctlie.jl` assertions are rewritten; the full suite is green via `ct-dev-mcp` (`get_test_command` → run + `tee` → `generate_report`). -- [ ] `BREAKING.md` records the new contract. -- [ ] The two CTFlows issues are filed and linked from `BREAKING.md`. +- [x] `BREAKING.md` records the new contract. +- [x] The two CTFlows issues are filed and linked from `BREAKING.md`. --- diff --git a/src/OptimalControl.jl b/src/OptimalControl.jl index 8198aa62..0feab05b 100644 --- a/src/OptimalControl.jl +++ b/src/OptimalControl.jl @@ -80,6 +80,9 @@ include(joinpath(@__DIR__, "imports", "examodels.jl")) include(joinpath(@__DIR__, "imports", "ad.jl")) # include(joinpath(@__DIR__, "imports", "redefine.jl")) +# v2.0 deprecation shims +include(joinpath(@__DIR__, "deprecated.jl")) + # helpers include(joinpath(@__DIR__, "helpers", "kwarg_extraction.jl")) include(joinpath(@__DIR__, "helpers", "print.jl")) diff --git a/src/deprecated.jl b/src/deprecated.jl new file mode 100644 index 00000000..00173597 --- /dev/null +++ b/src/deprecated.jl @@ -0,0 +1,82 @@ +# ============================================================================ +# v2.0 deprecation shims +# ============================================================================ +# This file is a deliberate, temporary piracy layer. It extends functions and +# types owned by CTFlows, CTModels, Base and LinearAlgebra so that removed v2.0 +# spellings fail with a CTBase.PreconditionError naming their v2.1.0-beta +# replacement, instead of a bare UndefVarError or MethodError. +# +# Not shimmed (Julia cannot dispatch on these cases or they are handled +# upstream): +# - Flow(ocp, u, g, mu) upstream PreconditionError in CTFlows/building.jl +# - augment=true keyword name, not a dispatch target +# - autonomous=/variable=/inplace= Data constructors keyword renames +# - @Lie ... autonomous= macro-time IncorrectArgument in CTLie +# +# Included after src/imports/ and before src/helpers/. +# ============================================================================ + +using Base: time, success +using LinearAlgebra: ⋅ + +export Lie, ⋅, HamiltonianLift + +function _deprecated(old, new, ctx = nothing) + return PreconditionError( + "`$old` is deprecated"; + reason = "this spelling was removed in v2.1.0-beta", + suggestion = "use $new", + context = ctx, + ) +end + +# Differential geometry ------------------------------------------------------- + +function Lie(X, f) + throw(_deprecated("Lie(X, f) / Lie(X, Y)", "ad(X, f) or ad(X, Y)")) +end + +LinearAlgebra.dot(X::AbstractVectorField, f::Function) = + throw(_deprecated("X \\cdot f", "ad(X, f)")) + +function HamiltonianLift(args...) + throw(_deprecated( + "HamiltonianLift", + "Lift(f) for a plain function, or CTLie.LiftedHamiltonianFunction", + )) +end + +# Removed accessors ----------------------------------------------------------- + +Base.time(m::Model) = + throw(_deprecated("time(ocp)", "times(ocp)")) + +Base.time(sol::AbstractSolution) = + throw(_deprecated("time(sol)", "time_grid(sol)")) + +Base.success(sol::AbstractSolution) = + throw(_deprecated("success(sol)", "successful(sol)")) + +# Flow constructor ------------------------------------------------------------ + +CTFlows.Flows.Flow(f::Function) = + throw(_deprecated( + "Flow(f::Function)", + "Flow(VectorField(f)), Flow(Hamiltonian(f)), or Flow(HamiltonianVectorField(f))", + )) + +# Flow calling convention ----------------------------------------------------- + +function (f::CTFlows.Flows.AbstractHamiltonianFlow)(t0::Real, x0, p0, tf::Real, variable) + throw(_deprecated( + "f(t0, x0, p0, tf, lambda)", + "f(t0, x0, p0, tf; variable=lambda)", + )) +end + +function (f::CTFlows.Flows.AbstractStateFlow)(t0::Real, x0, tf::Real, variable) + throw(_deprecated( + "f(t0, x0, tf, lambda)", + "f(t0, x0, tf; variable=lambda)", + )) +end diff --git a/test/suite/flows/test_flow_api.jl b/test/suite/flows/test_flow_api.jl index 5f01d381..6e00645d 100644 --- a/test/suite/flows/test_flow_api.jl +++ b/test/suite/flows/test_flow_api.jl @@ -270,6 +270,57 @@ function test_flow_api() f = Flow(build_nonfixed(), (x, p, v) -> p[2]) Test.@test_throws OptimalControl.PreconditionError f(T0, X0, P0, TF) end + + Test.@testset "deprecated v2.0 flow shims" begin + # The old Flow(f::Function) constructor. + e = try + Flow(x -> [x[2], -x[1]]) + catch err + err + end + Test.@test e isa OptimalControl.PreconditionError + Test.@test occursin("Flow(f::Function)", e.msg) + Test.@test occursin("Flow(VectorField", e.suggestion) + + # 5-positional Hamiltonian flow call. + f = Flow(ocp, (x, p) -> p[2]) + e = try + f(T0, X0, P0, TF, 0.0) + catch err + err + end + Test.@test e isa OptimalControl.PreconditionError + Test.@test occursin("f(t0, x0, p0, tf, lambda)", e.msg) + Test.@test occursin("f(t0, x0, p0, tf; variable=lambda)", e.suggestion) + + # 4-positional State flow call. + f_closed = Flow(ocp, ClosedLoop(x -> 0.0)) + e = try + f_closed(T0, X0, TF, 0.0) + catch err + err + end + Test.@test e isa OptimalControl.PreconditionError + Test.@test occursin("f(t0, x0, tf, lambda)", e.msg) + Test.@test occursin("f(t0, x0, tf; variable=lambda)", e.suggestion) + + # time and success on the OCP and on a Solution. + traj = f((T0, TF), X0, P0) + for (thunk, expected, suggestion) in ( + (() -> Base.time(ocp), "time(ocp)", "times(ocp)"), + (() -> Base.time(traj), "time(sol)", "time_grid(sol)"), + (() -> Base.success(traj), "success(sol)", "successful(sol)"), + ) + e = try + thunk() + catch err + err + end + Test.@test e isa OptimalControl.PreconditionError + Test.@test occursin(expected, e.msg) + Test.@test occursin(suggestion, e.suggestion) + end + end end end diff --git a/test/suite/reexport/test_ctlie.jl b/test/suite/reexport/test_ctlie.jl index d6ead3e6..e251a476 100644 --- a/test/suite/reexport/test_ctlie.jl +++ b/test/suite/reexport/test_ctlie.jl @@ -22,6 +22,7 @@ using Test: Test using OptimalControl # using is mandatory since we test exported symbols using CTLie: CTLie using CTBase: CTBase +using LinearAlgebra: LinearAlgebra include(joinpath(@__DIR__, "..", "..", "helpers", "reexport.jl")) using .ReexportUtils: reexports, imports, is_exported @@ -76,12 +77,13 @@ function test_ctlie() Test.@test imports(OptimalControl, :LiftedHamiltonianFunction, CTLie) end - Test.@testset "Removed API" begin - # `Lie` was renamed to `ad`; `⋅` was dropped with no replacement. - # Both must be gone from the public surface. - Test.@test !is_exported(OptimalControl, :Lie) - Test.@test !is_exported(OptimalControl, :⋅) - Test.@test !isdefined(OptimalControl, :HamiltonianLift) + Test.@testset "Removed API is now shimmed" begin + # `Lie` was renamed to `ad`; `⋅` and `HamiltonianLift` were removed. + # All three now throw PreconditionErrors naming their replacement. + Test.@test is_exported(OptimalControl, :Lie) + Test.@test is_exported(OptimalControl, :⋅) + Test.@test isdefined(OptimalControl, :HamiltonianLift) + Test.@test getfield(OptimalControl, :⋅) === LinearAlgebra.dot end # ==================================================================== diff --git a/test/suite/reexport/test_deprecated.jl b/test/suite/reexport/test_deprecated.jl new file mode 100644 index 00000000..aa0b396f --- /dev/null +++ b/test/suite/reexport/test_deprecated.jl @@ -0,0 +1,55 @@ +# ============================================================================ +# Deprecation shims tests +# ============================================================================ +# Every v2.0 spelling removed in v2.1.0-beta must fail loudly with a +# CTBase.PreconditionError that names its replacement. + +module TestDeprecated + +using Test: Test +using OptimalControl +using LinearAlgebra: LinearAlgebra + +const VERBOSE = isdefined(Main, :TestOptions) ? Main.TestOptions.VERBOSE : true +const SHOWTIMING = isdefined(Main, :TestOptions) ? Main.TestOptions.SHOWTIMING : true + +function check_shim(thunk, expected_old, expected_new) + e = try + thunk() + nothing + catch err + err + end + Test.@test e isa OptimalControl.PreconditionError + Test.@test occursin("deprecated", e.msg) + Test.@test occursin(expected_old, e.msg) + Test.@test occursin(expected_new, e.suggestion) + return nothing +end + +function test_deprecated() + Test.@testset "Deprecated v2.0 shims" verbose = VERBOSE showtiming = SHOWTIMING begin + X = VectorField(x -> [x[2], -x[1]]) + f = x -> x[1]^2 + x[2]^2 + Y = VectorField(x -> [x[1], x[2]]) + + Test.@testset "Lie" begin + check_shim(() -> Lie(X, f), "Lie", "ad") + check_shim(() -> Lie(X, Y), "Lie", "ad") + end + + Test.@testset "dot \\cdot" begin + Test.@test getfield(OptimalControl, :⋅) === LinearAlgebra.dot + check_shim(() -> X ⋅ f, "\\cdot", "ad") + end + + Test.@testset "HamiltonianLift" begin + check_shim(() -> HamiltonianLift(), "HamiltonianLift", "Lift") + check_shim(() -> HamiltonianLift(X), "HamiltonianLift", "Lift") + end + end +end + +end # module + +test_deprecated() = TestDeprecated.test_deprecated()