Add Claude code review skills - and apply them#423
Open
tmigot wants to merge 12 commits into
Open
Conversation
- polygon: :contype :linear -> :general (has 1225 nonlinear constraints) - polygon: :is_feasible false -> missing (feasible set is non-empty) - polygon1: :is_feasible false -> true (x0 satisfies all constraints) - polygon2: :is_feasible false -> missing (feasible set is non-empty) - clplatea/b/c PureJuMP: add max(..., 3) guard to p = floor(Int, sqrt(n)) - clplatea/b/c Meta: update get_*_nvar getter to use max(floor(Int,sqrt(n)),3)^2 The PureJuMP and Meta getter were missing the minimum grid size p=3 that ADNLPProblems already enforced, causing nvar mismatches at n=5 (ADNLP: 9, JuMP: 4). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
catenary: add init=zero(T) to the inner sum over 1:N in the objective. With n=6 (enforced minimum), N=div(6,3)-2=0, so the range 1:0 is empty and Julia's sum over a generator with no init throws ArgumentError. clnlbeam: enforce n>=6 before computing N=div(n-3,3) so N>=1 is guaranteed. Without the guard, n=5 gives N=0 and h=1//0 (division by zero). Also save n_org and pass it to @adjust_nvar_warn so the warning message reports the original user-supplied value. Update all Meta getters to use max(n,6) so they agree with the constructor. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
meta_is_feasible logic overhaul: - :is_feasible is problem-level, not x0-level. meta=true with an infeasible x0 is correct; was wrongly CROSS:FAIL, giving 27 false positives on hs problems. - meta=true + actual=missing -> CROSS:INFO (cannot refute from x0 alone) - meta=true + actual=false -> CROSS:FAIL (provably empty feasible set) - meta=false + actual=true -> CROSS:FAIL (x0 feasible, clear contradiction) - meta=false + actual=missing -> CROSS:WARN (consider changing to missing) - meta=false + actual=false -> CROSS:OK New static analysis rules: - ADNLPProblems: warn on sum without init in scalable problems where N can be 0 (catenary-style crash pattern) - ADNLPProblems: warn when n-adjustment guard in ADNLP is absent from PureJuMP or Meta getter (clplate-style inconsistency) - Meta: warn on :is_feasible => false unless provably infeasible (polygon-style bug) Known exceptions: added note on :is_feasible => true with infeasible x0 being correct for constrained HS problems where feasibility is known at problem level. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds repository-local Claude “code review skill” docs and applies related conventions across the OptimizationProblems problem definitions: standardizing n-adjustment warnings, improving JuMP variable start-value initialization, and tightening meta/documentation consistency.
Changes:
- Replace ad-hoc
@warnminimum-nguards with the standardized@adjust_nvar_warn(...)pattern across manyPureJuMPandADNLPProblemsproblems. - Add/standardize JuMP variable
startvalues (or equivalent start-value setting) for consistency with AD implementations. - Update multiple
Metaentries and docs (includingobjtypedocs) and add new.claude/skills/*review skill documents.
Reviewed changes
Copilot reviewed 159 out of 159 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test-scalable.jl | Adds an assertion in the scalable-problem test error path (but currently contains a variable-name bug). |
| src/PureJuMP/tquartic.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/torsion.jl | Tightens n typing in signature for consistency. |
| src/PureJuMP/tointgss.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/structural.jl | Adds explicit JuMP start values to variables. |
| src/PureJuMP/sparsqur.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/sparsine.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/sinquad.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/scosine.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/schmvett.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/sbrybnd.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/polygon3.jl | Adds explicit JuMP start values to variables. |
| src/PureJuMP/polygon2.jl | Adds explicit JuMP start values to variables. |
| src/PureJuMP/polygon1.jl | Adds explicit JuMP start values to variables. |
| src/PureJuMP/penalty3.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/penalty2.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/nondquar.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/nondia.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/noncvxun.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/noncvxu2.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/ncb20b.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/ncb20.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/nazareth.jl | Allows forwarding kwargs... in signature for compatibility. |
| src/PureJuMP/morebv.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/minsurf.jl | Tightens n typing in signature for consistency. |
| src/PureJuMP/liarwhd.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/indef_mod.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/hovercraft1d.jl | Adds explicit JuMP start values to variables. |
| src/PureJuMP/gulf.jl | Makes problem fixed-size in JuMP (3 parameters) and updates variable definition; docstring updated but is misleading. |
| src/PureJuMP/genrose.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/genrose_nash.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/genbroydentri.jl | Allows forwarding kwargs... in signature for compatibility. |
| src/PureJuMP/genbroydenb.jl | Allows forwarding kwargs... in signature for compatibility. |
| src/PureJuMP/freuroth.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/fletchcr.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/fletcbv3_mod.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/fletcbv2.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/extrosnb.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/errinros_mod.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/engval1.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/eg2.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/edensch.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/curly.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/cragglvy2.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/cragglvy.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/cosine.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/clplatec.jl | Clamps plate discretization parameter to a minimum and adjusts n accordingly. |
| src/PureJuMP/clplateb.jl | Clamps plate discretization parameter to a minimum and adjusts n accordingly. |
| src/PureJuMP/clplatea.jl | Clamps plate discretization parameter to a minimum and adjusts n accordingly. |
| src/PureJuMP/chnrosnb_mod.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/broyden7d.jl | Allows forwarding kwargs... in signature for compatibility. |
| src/PureJuMP/browngen2.jl | Adds n adjustment + warning (and forwards kwargs...). |
| src/PureJuMP/browngen1.jl | Adds n adjustment + warning (and forwards kwargs...). |
| src/PureJuMP/brownden.jl | Renames/standardizes size keyword (n) and uses @adjust_nvar_warn. |
| src/PureJuMP/bdqrtic.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/auglag.jl | Allows forwarding kwargs... in signature for compatibility. |
| src/PureJuMP/arwhead.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/PureJuMP/argtrig.jl | Simplifies signature to just n (drops m) and removes related guard logic. |
| src/OptimizationProblems.jl | Updates documented allowed objtype values to include :least_squares. |
| src/Meta/watson.jl | Fixes notes formatting and spelling. |
| src/Meta/triangle_turtle.jl | Adds/expands problem notes and provenance text. |
| src/Meta/triangle_pacman.jl | Adds/expands problem notes and provenance text. |
| src/Meta/triangle_deer.jl | Adds/expands problem notes and provenance text. |
| src/Meta/tetra_hook.jl | Adds/expands problem notes and provenance text. |
| src/Meta/tetra_gear.jl | Adds/expands problem notes and provenance text. |
| src/Meta/tetra_foam5.jl | Adds/expands problem notes and provenance text. |
| src/Meta/tetra_duct20.jl | Adds/expands problem notes and provenance text. |
| src/Meta/tetra_duct15.jl | Adds/expands problem notes and provenance text. |
| src/Meta/tetra_duct12.jl | Adds/expands problem notes and provenance text. |
| src/Meta/srosenbr.jl | Adjusts nvar getter to enforce the intended even-dimension structure. |
| src/Meta/sparsqur.jl | Updates nvar getter to enforce minimum size. |
| src/Meta/sparsine.jl | Updates nvar getter to enforce minimum size. |
| src/Meta/rosenbrock.jl | Expands notes/provenance for the Rosenbrock problem. |
| src/Meta/polygon2.jl | Updates feasibility metadata value. |
| src/Meta/polygon1.jl | Updates feasibility metadata value. |
| src/Meta/polygon.jl | Adjusts constraint-type and feasibility metadata. |
| src/Meta/ncb20b.jl | Updates nvar getter to enforce minimum size. |
| src/Meta/ncb20.jl | Updates nvar getter to enforce minimum size. |
| src/Meta/nazareth.jl | Updates provenance classification (:origin). |
| src/Meta/marine.jl | Clamps n in nvar getter to the model’s minimum structure. |
| src/Meta/genbroydentri.jl | Updates provenance classification (:origin) but currently has an invalid multi-line URL string. |
| src/Meta/genbroydenb.jl | Updates provenance classification (:origin). |
| src/Meta/clplatec.jl | Updates nvar getter to match clamped plate minimum discretization. |
| src/Meta/clplateb.jl | Updates nvar getter to match clamped plate minimum discretization. |
| src/Meta/clplatea.jl | Updates nvar getter to match clamped plate minimum discretization. |
| src/Meta/clnlbeam.jl | Updates getters to clamp minimum size to a valid beam discretization. |
| src/Meta/broyden7d.jl | Updates provenance classification (:origin). |
| src/Meta/browngen2.jl | Updates provenance classification (:origin). |
| src/Meta/browngen1.jl | Updates provenance classification (:origin). |
| src/Meta/bearing.jl | Guards against non-positive nx/ny in nvar getter. |
| src/Meta/auglag.jl | Updates provenance classification (:origin). |
| src/Meta/aircrfta.jl | Refines :objtype classification. |
| src/ADNLPProblems/tquartic.jl | Standardizes n adjustment warning via @adjust_nvar_warn for NLP/NLS variants. |
| src/ADNLPProblems/tointgss.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/sparsqur.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/sparsine.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/sinquad.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/scosine.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/schmvett.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/sbrybnd.jl | Standardizes n adjustment warning via @adjust_nvar_warn for NLP/NLS variants. |
| src/ADNLPProblems/polygon.jl | Removes type-parameterized constraint signature and uses @views to avoid allocations. |
| src/ADNLPProblems/penalty3.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/penalty2.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/palmer8c.jl | Refactors residual to avoid per-call typed-array construction (allocation reduction). |
| src/ADNLPProblems/palmer7c.jl | Refactors residual to avoid per-call typed-array construction (allocation reduction). |
| src/ADNLPProblems/palmer6c.jl | Refactors residual to avoid per-call typed-array construction (allocation reduction). |
| src/ADNLPProblems/palmer5d.jl | Refactors residual + updates NLS model name to *-nls. |
| src/ADNLPProblems/palmer5c.jl | Refactors residual to avoid per-call typed-array construction (allocation reduction). |
| src/ADNLPProblems/palmer4c.jl | Refactors residual + updates NLS model name to *-nls. |
| src/ADNLPProblems/palmer3c.jl | Refactors residual to avoid per-call typed-array construction (allocation reduction). |
| src/ADNLPProblems/palmer2c.jl | Refactors residual to avoid per-call typed-array construction (allocation reduction). |
| src/ADNLPProblems/palmer1d.jl | Refactors residual to avoid per-call typed-array construction (allocation reduction). |
| src/ADNLPProblems/palmer1c.jl | Refactors residual to avoid per-call typed-array construction (allocation reduction). |
| src/ADNLPProblems/nondquar.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/nondia.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/noncvxun.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/noncvxu2.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/ncb20b.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/ncb20.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/nazareth.jl | Updates NLS model naming to *-nls. |
| src/ADNLPProblems/morebv.jl | Standardizes n adjustment warning via @adjust_nvar_warn for NLP/NLS variants. |
| src/ADNLPProblems/liarwhd.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/kowosb.jl | Removes silent n=4 override (but leaves n keyword present and now ignored). |
| src/ADNLPProblems/indef_mod.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/hs37.jl | Removes unused nonlinear-constraint callback, relying on linear constraint specification. |
| src/ADNLPProblems/hs36.jl | Removes unused nonlinear-constraint callback, relying on linear constraint specification. |
| src/ADNLPProblems/genrose.jl | Standardizes n adjustment warning and refactors rosenbrock wrapper. |
| src/ADNLPProblems/genrose_nash.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/freuroth.jl | Standardizes n adjustment warning via @adjust_nvar_warn for NLP/NLS variants. |
| src/ADNLPProblems/fletchcr.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/fletcbv3_mod.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/fletcbv2.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/extrosnb.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/errinros_mod.jl | Standardizes n adjustment warning via @adjust_nvar_warn (multiple methods). |
| src/ADNLPProblems/engval1.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/eg2.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/edensch.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/curly30.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/curly20.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/curly10.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/curly.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/cragglvy2.jl | Standardizes n adjustment warning via @adjust_nvar_warn for NLP/NLS variants. |
| src/ADNLPProblems/cragglvy.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/cosine.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/controlinvestment.jl | Renames initial point variable for clarity (xi → x0). |
| src/ADNLPProblems/clnlbeam.jl | Clamps n to a minimum valid discretization and warns via @adjust_nvar_warn. |
| src/ADNLPProblems/chnrosnb_mod.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/catenary.jl | Adds init = zero(T) to prevent empty-reduction issues in scalable sums. |
| src/ADNLPProblems/browngen2.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/browngen1.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/brownden.jl | Renames/standardizes size keyword (n) and updates NLS residual implementation. |
| src/ADNLPProblems/bdqrtic.jl | Standardizes n adjustment warning via @adjust_nvar_warn for NLP/NLS variants. |
| src/ADNLPProblems/arwhead.jl | Standardizes n adjustment warning via @adjust_nvar_warn. |
| src/ADNLPProblems/argauss.jl | Simplifies initial-point construction (x0) and removes redundant variable. |
| docs/src/contributing.md | Clarifies where documentation belongs and enforces meta getter requirements. |
| .claude/skills/review-purejump/SKILL.md | Adds a detailed PureJuMP review “skill” document. |
| .claude/skills/review-problem/SKILL.md | Adds a comprehensive cross-file (AD/JuMP/Meta) review “skill” document. |
| .claude/skills/review-meta/SKILL.md | Adds a detailed Meta-file review “skill” document. |
| .claude/skills/review-adnlpproblems/SKILL.md | Adds a detailed ADNLPProblems review “skill” document. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
8
to
10
| function kowosb(::Val{:nlp}; n::Int = default_nvar, type::Type{T} = Float64, kwargs...) where {T} | ||
| n = 4 | ||
| m = 11 | ||
| y = [ |
Comment on lines
45
to
47
| function kowosb(::Val{:nls}; n::Int = default_nvar, type::Type{T} = Float64, kwargs...) where {T} | ||
| n = 4 | ||
| m = 11 | ||
| y = [ |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.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
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.
No description provided.