Conversation
Each solver can now set the step status (accepted or rejected) for use in a callback. This is particularly useful to perform quasi-Newton updates.
dpo
requested review from
Copilot and
tmigot
and removed request for
Copilot
There was a problem hiding this comment.
Pull request overview
This PR extends GenericExecutionStats with a per-iteration step_status (and step_status_reliable) so solvers can report whether the most recent step was accepted/rejected for downstream callbacks (e.g., quasi-Newton updates).
Changes:
- Added
step_status/step_status_reliablefields and aset_step_status!setter, plus supportingSTEP_STATUSESutilities. - Extended
statsgetfieldto render:step_statusas a human-readable string. - Updated the NLPModels extension constructor and expanded tests to cover the new setter and reliability flag.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/stats.jl |
Adds step-status storage, validation, setter, and formatted access via statsgetfield. |
ext/SolverCoreNLPModelsExt.jl |
Threads the new step_status keyword through the NLPModels-specific constructor. |
test/test-stats.jl |
Adds tests for invalid step statuses and for the new reliability flag. |
Comments suppressed due to low confidence (1)
ext/SolverCoreNLPModelsExt.jl:85
- In the NLPModels extension constructor you validate
statusviaSolverCore.check_status(status), but the newly addedstep_statuskeyword is not validated. This allows invalidstep_statusvalues to be stored at construction time and later crash when rendering/logging (e.g.,getStepStatusindexingSTEP_STATUSES). Consider callingSolverCore.check_step_status(step_status)here as well (mirroring the status check).
step_status::Symbol = :unknown,
elapsed_time::Real = Inf,
solver_specific::Dict{Symbol, Tsp} = Dict{Symbol, Any}(),
) where {T, S, V, Tsp}
SolverCore.check_status(status)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| set_iter!, | ||
| set_step_status!, | ||
| set_time!, | ||
| broadcast_solver_specific!, |
There was a problem hiding this comment.
show_step_statuses() and getStepStatus() are introduced as public-facing helpers (docstring + analogous to show_statuses/getStatus), but they are not exported. This makes the new step-status API harder to discover and inconsistent with the existing status helpers. Consider exporting show_step_statuses (and optionally getStepStatus) alongside set_step_status!, or removing the public docstring if these are intended to remain internal-only.
There was a problem hiding this comment.
show_step_statuses() could be exported.
| elseif name == :step_status | ||
| v = getStepStatus(stats) | ||
| t = String |
There was a problem hiding this comment.
statsgetfield now supports :step_status, but statshead(line) uses the headsym dict to map symbols to headers and currently has no entry for :step_status. As a result, requesting a stats table that includes :step_status will throw a KeyError in statshead. Add a :step_status entry to headsym (or make statshead handle missing header symbols gracefully).
There was a problem hiding this comment.
Indeed, An adaptation is missing in const headsym above.
There was a problem hiding this comment.
Is it really? The headsym dict is used when printing stats after the solver has returned. At that point, there's not much sense in showing whether a step was accepted or not, is there? The step status is just used during the iterations.
|
src/stats.jl
Outdated
| - `multipliers_L`: The Lagrange multipliers wrt to the lower bounds on the variables (default: an uninitialized vector like `nlp.meta.x0` if there are bounds, or a zero-length vector if not); | ||
| - `multipliers_U`: The Lagrange multipliers wrt to the upper bounds on the variables (default: an uninitialized vector like `nlp.meta.x0` if there are bounds, or a zero-length vector if not); | ||
| - `iter`: The number of iterations computed by the solver (default: `-1`); | ||
| - `step_status`: The status of the most recently computed step (`:unknown`, `:accepted` or `:rejected`); |
There was a problem hiding this comment.
see show_step_statuses() for a list
| elseif name == :step_status | ||
| v = getStepStatus(stats) | ||
| t = String |
There was a problem hiding this comment.
Indeed, An adaptation is missing in const headsym above.
| set_iter!, | ||
| set_step_status!, | ||
| set_time!, | ||
| broadcast_solver_specific!, |
There was a problem hiding this comment.
show_step_statuses() could be exported.
2 participants
Each solver can now set the step status (accepted or rejected) for use in a callback.
This is particularly useful to perform quasi-Newton updates.
Related issues
There is no related issue.
Checklist