add ScaledModel - #123
add ScaledModel#123frapac wants to merge 7 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #123 +/- ##
==========================================
- Coverage 97.29% 92.87% -4.42%
==========================================
Files 6 7 +1
Lines 886 1067 +181
==========================================
+ Hits 862 991 +129
- Misses 24 76 +52 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
| the gradient and the Jacobian evaluated at the initial point ``x0``. | ||
|
|
||
| """ | ||
| struct ScaledModel{T, S, M} <: NLPModels.AbstractNLPModel{T, S} |
There was a problem hiding this comment.
and same comment throughout the file
|
The linear and nonlinear API for the constraints have been implemented. |
dpo
left a comment
dpo
left a comment
There was a problem hiding this comment.
Thank you! I think we can use this in multiple places. I just have a few comments to make the code more explicit.
| end | ||
| end | ||
|
|
||
| function _set_jacobian_scaling!(Jx, Ji, Jj, cons) |
There was a problem hiding this comment.
The name cons suggests "constraint" (values). But that's not what it is, is it?
There was a problem hiding this comment.
Indeed, the name scaling is more appropriate
Co-authored-by: Maxence Gollier <134112149+MaxenceGollier@users.noreply.github.com>
|
@MaxenceGollier Does this work for you now? |
|
@dpo yes. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved critical and moderate correctness issues require remediation before approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 3
Open (6)
What changed in this PR
Adds a ScaledModel wrapper for scaling NLP objectives, constraints, Jacobians, and Hessians.
Changes:
- Implements scaled model operations.
- Registers the model and tests.
- Extends test metadata and API coverage.
| File | Summary and review findings |
|---|---|
test/runtests.jl |
Registers scaled-model tests. |
test/nlp/simple-model.jl |
Extends test metadata. |
test/nlp/scaled-model.jl |
Adds API tests. Moderate (2 votes): tests use identity scaling and do not cover non-unit scaling, split Jacobian products, or multiplier Hessians. |
src/scaled-model.jl |
Implements scaling. Critical (3 votes): jprod_nln! delegates to the linear product. Critical (3 votes): passes unscaled y to the constraint Hessian. Critical (2 votes): CPU-only zeros allocations are incompatible with custom array types on lines 112 and 118. Moderate (3 votes): counters are neither delegated nor updated. Moderate (1 vote): y0 uses an incorrect multiplier transformation. Nit (3 votes): bound descriptions on lines 49 and 57 use ≥ instead of ≤. |
src/NLPModelsModifiers.jl |
Registers the new model implementation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # Get scaling for linear and nonlinear constraints. | ||
| scaling_cons_lin = scaling_cons[nlp.meta.lin] | ||
| scaling_cons_nln = scaling_cons[nlp.meta.nln] | ||
| scaling_jac_lin = zeros(T, nlp.meta.lin_nnzj) |
| return ScaledModel( | ||
| nlp, | ||
| meta, | ||
| NLPModels.Counters(), |
| @testset "ScaledModel NLP tests" begin | ||
| @testset "API" for T in [Float64, Float32], M in [NLPModelMeta, SimpleNLPMeta] | ||
| original_nlp = SimpleNLPModel(T, M) | ||
| nlp = ScaledModel(original_nlp) |
|
The remaining comments have been addressed. Let me know if you any remaining feedback before merging this branch in |
Following a suggestion by @dpo