refactor(DOCP): dispatch-based contract, no builders/callables#155
Merged
Conversation
Replace the closure-factory → builder-accessor → callable indirection with a
thin DiscretizedModel and named, problem-first contract methods.
CTSolvers:
- DiscretizedModel slimmed to {ocp, discretizer, cache} (cache <: CTBase.Core.AbstractCache)
- AbstractDiscretizer moved into DOCP (DOCP/abstract_discretizer.jl)
- new DOCP/contract.jl: discretize stub (NotImplemented), implemented downstream
- Optimization.build_model / build_solution become NotImplemented stubs
(implemented by dispatch on (problem, modeler) in the providing package)
- remove Optimization/builders.jl, Optimization/contract.jl, DOCP/contract_impl.jl
and all AbstractBuilder / get_*_builder boilerplate
- remove modeler callables; modelers now only carry options + metadata
Part C — solvers: drop the (solver)(nlp) callable in favor of
CommonSolve.solve(nlp, solver) methods (generic NotImplemented stub in core,
typed methods in the Ipopt/Knitro/MadNLP/MadNCL/Uno extensions).
Behavior-preserving reorganization; tests updated in a follow-up commit.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rewrite tests around the new build_model / build_solution contract and CommonSolve.solve(nlp, solver), removing all builder/accessor and callable usages: - test_docp / test_optimization / test_error_cases: fake discretizer + cache, contract by dispatch, NotImplemented stubs - shared problems/*.jl: OptimizationProblem implements build_model/build_solution - test_real_problems / test_end_to_end: build via Optimization.build_model(prob, init, modeler) - solver tests: (solver)(nlp) -> CommonSolve.solve(nlp, solver) - extension tests: builder extraction -> build_model, solver callable -> CommonSolve.solve Full suite green: 2442 passed, 0 failed, 0 errored. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ractOptimizationProblem The problem argument is always an AbstractOptimizationProblem (same module), so constrain the generic stubs accordingly. The modeler stays untyped here: its type (Modelers.AbstractNLPModeler) lives in the Modelers submodule, which is loaded after Optimization and depends on it, so Optimization cannot reference it. Fine- grained modeler typing already exists downstream (DOCP/building.jl and the concrete CTDirect methods). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
Context
The discretization path went through a closure-factory → builder-accessor → callable indirection: a
(discretizer)(ocp)callable (CTDirect) returned 4 closures wrapped in aDiscretizedModel{4 builders}, which modelers reopened viaget_*_builderaccessors. Problems: implicit temporal coupling (exa_getter = nothingmutated as a side effect), scattered contract, ~120 lines ofAbstractBuilderboilerplate, struct coupled to backends (4 type-params), andAbstractDiscretizerowned by CTDirect while CTSolvers used it.This PR replaces that with direct multiple dispatch, problem-first, no callables, no builders. It makes the DOCP boundary consistent with the upcoming ODE
Integratorsboundary (sameCommonSolve.solve(prob, alg)idiom).Behaviour-preserving reorganization — solving semantics unchanged; only internal structures and contract method names move.
Changes
Core
DiscretizedModelslimmed to{ocp, discretizer, cache}withcache <: CTBase.Core.AbstractCacheAbstractDiscretizermoved intoDOCP/(DOCP/abstract_discretizer.jl)DOCP/contract.jl:discretizestub (NotImplemented), implemented downstream (CTDirect)Optimization.build_model/build_solutionbecomeNotImplementedstubs, implemented by dispatch on(problem, modeler)in the providing packageOptimization/builders.jl,Optimization/contract.jl,DOCP/contract_impl.jland allAbstractBuilder/get_*_builderboilerplatePart C — solvers
(solver)(nlp)callable in favor ofCommonSolve.solve(nlp, solver)methods (genericNotImplementedstub in core, typed methods in the Ipopt/Knitro/MadNLP/MadNCL/Uno extensions)Tests — rewritten around the new contract; full suite green (2442 passed, 0 failed, 0 errored).
Downstream
Requires the companion CTDirect PR (implements
CTSolvers.discretize/build_model/build_solutionforCollocation/DirectShooting). Merge CTSolvers first, then CTDirect.Out of scope (separate increments)
NLPModels/SolverCorehard→weak demotion (Part B)Integratorssubmodule import from CTFlows🤖 Generated with Claude Code