Skip to content

Add interface for smooth problems#305

Closed
MaxenceGollier wants to merge 4 commits intoJuliaSmoothOptimizers:masterfrom
MaxenceGollier:smooth-problems
Closed

Add interface for smooth problems#305
MaxenceGollier wants to merge 4 commits intoJuliaSmoothOptimizers:masterfrom
MaxenceGollier:smooth-problems

Conversation

@MaxenceGollier
Copy link
Collaborator

@MaxenceGollier MaxenceGollier commented Mar 2, 2026

Copilot AI review requested due to automatic review settings March 2, 2026 21:51
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a “smooth problem” interface so the library’s solvers can be called directly on unregularized AbstractNLPModel / AbstractNLSModel inputs by automatically wrapping them in a Regularized*Model with a null regularizer (supporting issue #302’s goal of running solvers on unregularized problems).

Changes:

  • Added new tests exercising “smooth” (unregularized) NLP/NLS calls for multiple solvers.
  • Extended test harness to use SolverTest and to include the new smooth test file.
  • Added convenience overloads (via @eval) so R2/R2N/R2DH/TR/TRDH accept AbstractNLPModel and LM/LMTR accept AbstractNLSModel.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.

File Description
src/utils.jl Adds smooth-problem overloads that wrap inputs with a null regularizer before calling existing regularized entrypoints.
test/test-smooth.jl New testsets validating smooth (unregularized) NLP and NLS behavior for multiple solvers.
test/runtests.jl Imports SolverTest and includes the new smooth test file in the test suite.
Project.toml Adds SolverTest as a test dependency and constrains its compatible version.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/utils.jl Outdated
Comment on lines +213 to +214
reg_nls = RegularizedNLSModel(nlp, h)
$solver(reg_nls; kwargs...)
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The NLS smooth-problem overload uses RegularizedNLSModel, but the rest of this package’s NLS path (e.g. LM(nls, h, options, ...)) wraps NLS models with RegularizedNLPModel. To avoid relying on a type that isn’t otherwise used here (and may not exist in the current dependency set), consider switching this wrapper to RegularizedNLPModel(nlp, h) for consistency with the existing LM/LMTR constructors.

Suggested change
reg_nls = RegularizedNLSModel(nlp, h)
$solver(reg_nls; kwargs...)
reg_nlp = RegularizedNLPModel(nlp, h)
$solver(reg_nlp; kwargs...)

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A RegularizedNLSModel <: AbstractRegularizedNLPModel so this is fine with the API from RegularizedProblems though i agree that the above subtyping is odd.

Comment on lines +193 to +201
for solver in [
:R2,
:R2N,
:R2DH,
:TR,
:TRDH,
]
@eval $solver(nlp::AbstractNLPModel{T, V}; kwargs...) where{T, V} = begin
h = NullRegularizer(T)
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue/PR description example for #302 expects users to be able to work with plain AbstractNLPModels via the solver objects (solver = R2NSolver(nlp) and solve!(solver, nlp, stats)), but this PR only adds function-style convenience constructors (R2(nlp), TR(nlp), etc.). As-is, R2NSolver (and similar solver structs) still only accept AbstractRegularizedNLPModel, so the example workflow won’t work without additional overloads.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm actually, it would be risky to add an initialization of a reg_nlp directly in those I think.

Now that I think of it we could have a constructor RegularizedNLPModel(nlp) that returns a RegularizedNLPModel(nlp, NullRegularizer).

@MaxenceGollier
Copy link
Collaborator Author

Closing following the discussion with @dpo.

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.

Allow smooth box constrained problems

2 participants