Simplify the implementation of jprod! by dpo · Pull Request #564 · JuliaSmoothOptimizers/NLPModels.jl

dpo · 2026-04-17T01:29:00Z

I find jprod!() unnecessarily complicated.

Copilot

Pull request overview

This PR refactors jprod!() in the NLP API to reduce branching by using short-circuit calls for the linear and nonlinear Jacobian-vector products.

Changes:

Replaced nested if/else logic in jprod! with && short-circuit calls.
Always passes sliced destinations (view(Jv, ...)) to jprod_lin! and jprod_nln!.

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

Copilot · 2026-04-17T01:32:38Z

+  get_nlin(nlp.meta) > 0 && jprod_lin!(nlp, x, v, view(Jv, get_lin(nlp.meta)))
+  get_nnln(nlp.meta) > 0 && jprod_nln!(nlp, x, v, view(Jv, get_nln(nlp.meta)))


jprod! now always calls jprod_lin!/jprod_nln! with view(Jv, ...). This removes the previous fast paths for the all-linear (get_nnln(meta)==0) and all-nonlinear (get_nlin(meta)==0) cases, and changes the type of the destination passed to downstream implementations from a plain Vector to a SubArray even when no remapping is needed. That can be a breaking change for model implementations that specialized jprod_lin!/jprod_nln! on concrete vectors (and it introduces extra indirection for the common contiguous/full-range case). Consider restoring the old branching (as done in cons! / jac_coord!) or conditionally passing Jv directly when the constraint set is purely linear or purely nonlinear.

Suggested change

get_nlin(nlp.meta) > 0 && jprod_lin!(nlp, x, v, view(Jv, get_lin(nlp.meta)))

get_nnln(nlp.meta) > 0 && jprod_nln!(nlp, x, v, view(Jv, get_nln(nlp.meta)))

if get_nnln(nlp.meta) == 0

jprod_lin!(nlp, x, v, Jv)

elseif get_nlin(nlp.meta) == 0

jprod_nln!(nlp, x, v, Jv)

else

jprod_lin!(nlp, x, v, view(Jv, get_lin(nlp.meta)))

jprod_nln!(nlp, x, v, view(Jv, get_nln(nlp.meta)))

end

Copilot · 2026-04-17T01:32:39Z

+  get_nlin(nlp.meta) > 0 && jprod_lin!(nlp, x, v, view(Jv, get_lin(nlp.meta)))
+  get_nnln(nlp.meta) > 0 && jprod_nln!(nlp, x, v, view(Jv, get_nln(nlp.meta)))


There are no tests covering the edge cases where all constraints are linear (nnln==0) or all constraints are nonlinear (nlin==0). Since this change alters how jprod! dispatches (passing views vs the full vector), adding targeted tests for those two scenarios would help prevent regressions and catch downstream dispatch/type issues.

Suggested change

get_nlin(nlp.meta) > 0 && jprod_lin!(nlp, x, v, view(Jv, get_lin(nlp.meta)))

get_nnln(nlp.meta) > 0 && jprod_nln!(nlp, x, v, view(Jv, get_nln(nlp.meta)))

ncon = get_ncon(nlp.meta)

nlin = get_nlin(nlp.meta)

nnln = get_nnln(nlp.meta)

if nlin > 0

if nlin == ncon

jprod_lin!(nlp, x, v, Jv)

else

jprod_lin!(nlp, x, v, view(Jv, get_lin(nlp.meta)))

end

end

if nnln > 0

if nnln == ncon

jprod_nln!(nlp, x, v, Jv)

else

jprod_nln!(nlp, x, v, view(Jv, get_nln(nlp.meta)))

end

end

github-actions · 2026-04-17T02:00:51Z

Package name	latest	stable
ADNLPModels
AdaptiveRegularization
AmplNLReader
BundleAdjustmentModels
CUTEst
CaNNOLeS
DCISolver
FletcherPenaltySolver
FluxNLPModels
JSOSolvers
JSOSuite
LLSModels
ManualNLPModels
NLPModelsIpopt
NLPModelsJuMP
NLPModelsKnitro
NLPModelsModifiers
NLPModelsTest
NLSProblems
PDENLPModels
PartiallySeparableNLPModels
PartiallySeparableSolvers
Percival
QuadraticModels
RegularizedOptimization
RegularizedProblems
SolverBenchmark
SolverTest
SolverTools

Copilot

Pull request overview

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

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

Copilot · 2026-04-17T12:51:04Z

+  get_nlin(nlp.meta) > 0 && cons_lin!(nlp, x, view(cx, get_lin(nlp.meta)))
+  get_nnln(nlp.meta) > 0 && cons_nln!(nlp, x, view(cx, get_nln(nlp.meta)))


cons! now always forwards cx to cons_lin!/cons_nln! as view(cx, meta.lin/meta.nln). When the problem is purely linear or purely nonlinear, this changes the argument type from the original cx (often a Vector/StridedVector) to a SubArray with index-vector indirection, which can break downstream model implementations that dispatch on Vector/StridedVector or use BLAS/external kernels requiring strided storage, and can add avoidable overhead. Consider restoring the fast-path branches for get_nnln(meta)==0 / get_nlin(meta)==0 (pass cx directly) and only using indexed views when both linear and nonlinear constraints are present.

Suggested change

get_nlin(nlp.meta) > 0 && cons_lin!(nlp, x, view(cx, get_lin(nlp.meta)))

get_nnln(nlp.meta) > 0 && cons_nln!(nlp, x, view(cx, get_nln(nlp.meta)))

nlin = get_nlin(nlp.meta)

nnln = get_nnln(nlp.meta)

if nnln == 0

cons_lin!(nlp, x, cx)

elseif nlin == 0

cons_nln!(nlp, x, cx)

else

cons_lin!(nlp, x, view(cx, get_lin(nlp.meta)))

cons_nln!(nlp, x, view(cx, get_nln(nlp.meta)))

end

Copilot · 2026-04-17T12:51:05Z

  if get_nlin(nlp.meta) > 0
-    if get_nnln(nlp.meta) == 0
-      jac_lin_coord!(nlp, x, vals)
-    else
-      lin_ind = 1:(get_lin_nnzj(nlp.meta))
-      jac_lin_coord!(nlp, x, view(vals, lin_ind))
-    end
+    lin_ind = 1:(get_lin_nnzj(nlp.meta))
+    jac_lin_coord!(nlp, x, view(vals, lin_ind))
  end
  if get_nnln(nlp.meta) > 0
-    if get_nlin(nlp.meta) == 0
-      jac_nln_coord!(nlp, x, vals)
-    else
-      nln_ind = (get_lin_nnzj(nlp.meta) + 1):(get_lin_nnzj(nlp.meta) + get_nln_nnzj(nlp.meta))
-      jac_nln_coord!(nlp, x, view(vals, nln_ind))
-    end
+    nln_ind = (get_lin_nnzj(nlp.meta) + 1):(get_lin_nnzj(nlp.meta) + get_nln_nnzj(nlp.meta))
+    jac_nln_coord!(nlp, x, view(vals, nln_ind))


jac_coord! now always passes view(vals, lin_ind/nln_ind) into jac_lin_coord!/jac_nln_coord! even when the Jacobian is entirely linear or entirely nonlinear. This changes the concrete type seen by model implementations from the original vals (commonly a Vector) to a SubArray, which may break downstream methods that dispatch on Vector/StridedVector or call routines requiring strided storage. Suggest keeping the previous single-block fast paths (call jac_*_coord! with vals directly when the other block is empty) and using views only for the mixed case.

Copilot · 2026-04-17T12:51:05Z

+  get_nlin(nlp.meta) > 0 && jprod_lin!(nlp, x, v, view(Jv, get_lin(nlp.meta)))
+  get_nnln(nlp.meta) > 0 && jprod_nln!(nlp, x, v, view(Jv, get_nln(nlp.meta)))


jprod! now always calls jprod_lin!/jprod_nln! with view(Jv, meta.lin/meta.nln). For purely linear or purely nonlinear problems this changes the Jv argument from the original array (often a Vector/StridedVector) to an indexed SubArray, which can break downstream implementations that dispatch on Vector/StridedVector or use BLAS/external kernels, and may add overhead compared to the previous direct call. Consider reinstating the get_nnln(meta)==0 / get_nlin(meta)==0 fast paths (pass Jv directly) and only using indexed views when both constraint types are present.

Suggested change

get_nlin(nlp.meta) > 0 && jprod_lin!(nlp, x, v, view(Jv, get_lin(nlp.meta)))

get_nnln(nlp.meta) > 0 && jprod_nln!(nlp, x, v, view(Jv, get_nln(nlp.meta)))

if get_nnln(nlp.meta) == 0

jprod_lin!(nlp, x, v, Jv)

elseif get_nlin(nlp.meta) == 0

jprod_nln!(nlp, x, v, Jv)

else

jprod_lin!(nlp, x, v, view(Jv, get_lin(nlp.meta)))

jprod_nln!(nlp, x, v, view(Jv, get_nln(nlp.meta)))

end

amontoison · 2026-05-01T18:39:08Z

@dpo The code is quite complicated because we got a performance regression wity the linear API (see #491).
The PRs #492 and #495 are related to this issue.

dpo · 2026-05-07T22:51:03Z

Oh I see, thank you. It's really too bad that views cause so much trouble. I'll close this then.

amontoison · 2026-05-07T23:42:43Z

Views could be fine as long as they are strided. If they are not strided, we can't use BLAS / LAPACK (CPU and GPU) and Julia dispatches to generic fallbacks.

Simplify the implementation of jprod!

18c6440

Copilot AI review requested due to automatic review settings April 17, 2026 01:29

Copilot started reviewing on behalf of dpo April 17, 2026 01:29 View session

Copilot AI reviewed Apr 17, 2026

View reviewed changes

Simplify cons! and jac_coord! accordingly

b06bc34

dpo requested a review from Copilot April 17, 2026 12:44

Copilot started reviewing on behalf of dpo April 17, 2026 12:45 View session

Copilot AI reviewed Apr 17, 2026

View reviewed changes

dpo closed this May 7, 2026

amontoison deleted the jprod_simple branch May 7, 2026 23:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Simplify the implementation of jprod!#564

Simplify the implementation of jprod!#564
dpo wants to merge 2 commits into
mainfrom
jprod_simple

dpo commented Apr 17, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

github-actions Bot commented Apr 17, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

amontoison commented May 1, 2026 •

edited

Loading

Uh oh!

dpo commented May 7, 2026

Uh oh!

amontoison commented May 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		get_nlin(nlp.meta) > 0 && jprod_lin!(nlp, x, v, view(Jv, get_lin(nlp.meta)))
		get_nnln(nlp.meta) > 0 && jprod_nln!(nlp, x, v, view(Jv, get_nln(nlp.meta)))

-  get_nlin(nlp.meta) > 0 && jprod_lin!(nlp, x, v, view(Jv, get_lin(nlp.meta)))
-  get_nnln(nlp.meta) > 0 && jprod_nln!(nlp, x, v, view(Jv, get_nln(nlp.meta)))
+  ncon = get_ncon(nlp.meta)
+  nlin = get_nlin(nlp.meta)
+  nnln = get_nnln(nlp.meta)
+  if nlin > 0
+    if nlin == ncon
+      jprod_lin!(nlp, x, v, Jv)
+    else
+      jprod_lin!(nlp, x, v, view(Jv, get_lin(nlp.meta)))
+    end
+  end
+  if nnln > 0
+    if nnln == ncon
+      jprod_nln!(nlp, x, v, Jv)
+    else
+      jprod_nln!(nlp, x, v, view(Jv, get_nln(nlp.meta)))
+    end
+  end

		get_nlin(nlp.meta) > 0 && cons_lin!(nlp, x, view(cx, get_lin(nlp.meta)))
		get_nnln(nlp.meta) > 0 && cons_nln!(nlp, x, view(cx, get_nln(nlp.meta)))

Uh oh!

Conversation

dpo commented Apr 17, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

amontoison commented May 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dpo commented May 7, 2026

Uh oh!

amontoison commented May 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Apr 17, 2026 •

edited

Loading

amontoison commented May 1, 2026 •

edited

Loading