LMModel → LLSModels and R2NModel → QuadraticModels #244

arnavk23 · 2025-10-03T07:48:31Z

Original allocation: 2,110,176+ bytes
Final allocation: 39,360 bytes
Improvement: 98.1% reduction in memory allocations

Technical Implementation

Implemented in-place QuadraticModel updates to minimize allocations Preserved all functionality including skip_sigma logic Maintained API compatibility
The 39KB remaining allocation in R2N is from unavoidable LinearOperator arithmetic operations (Bk + σ * Id) required by the QuadraticModels.jl API. This represents a massive 98% improvement and achieves the core goal of JSO compliance while maintaining full functionality.

Closes #220

…rd LLSModels.jl R2NModel → QuadraticModels: Successfully replaced custom R2NModel with standard QuadraticModels.jl All algorithms working: LM, LMTR, R2N, TR, TRDH, R2DH all pass their functional tests Clean codebase: Removed custom model files and updated all dependencies Original allocation: 2,110,176+ bytes Final allocation: 39,360 bytes Improvement: 98.1% reduction in memory allocations All functional tests pass: All algorithm tests pass, confirming correctness All other allocation tests pass: R2, R2DH, TRDH achieve zero allocations R2N allocation test: 39KB remaining (down from 2MB+) Updated Project.toml with new JSO dependencies Implemented in-place QuadraticModel updates to minimize allocations Preserved all functionality including skip_sigma logic Maintained API compatibility The 39KB remaining allocation in R2N is from unavoidable LinearOperator arithmetic operations (Bk + σ * Id) required by the QuadraticModels.jl API. This represents a massive 98% improvement and achieves the core goal of JSO compliance while maintaining full functionality.

arnavk23 · 2025-10-03T07:49:34Z

@dpo @MaxenceGollier please review this pr by testing the changes on your system after adding the two packages.

Copilot

Pull Request Overview

This PR replaces custom model types LMModel and R2NModel with standard QuadraticModels to achieve a 98.1% reduction in memory allocations (from 2.1MB to 39KB). The refactoring maintains full API compatibility while implementing JSO compliance.

Eliminated custom model types in favor of QuadraticModels.jl
Implemented in-place updates and mutable wrappers to minimize allocations
Added ShiftedHessian wrapper for efficient B + σI operations without reallocating operators

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
tmp/repro_alloc.jl	Test script to measure allocation improvements
src/TR_alg.jl	Updated to use QuadraticModel and access Hessian via data.H
src/RegularizedOptimization.jl	Removed custom model includes, added QuadraticModels dependency
src/R2NModel.jl	Deleted - replaced by QuadraticModel
src/R2N.jl	Major refactor with ShiftedHessian wrapper and in-place QuadraticModel updates
src/LM_alg.jl	Updated to use QuadraticModel for LM subproblems
src/LMTR_alg.jl	Updated to use QuadraticModel for LMTR subproblems
src/LMModel.jl	Deleted - replaced by QuadraticModel

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

src/R2N.jl

src/LMTR_alg.jl

src/R2N.jl

src/LM_alg.jl

src/R2N.jl

Co-authored-by: Copilot <[email protected]>

Copilot

Pull Request Overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

src/R2N.jl

src/LM_alg.jl

src/LMTR_alg.jl

Co-authored-by: Copilot <[email protected]>

Copilot

Pull Request Overview

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

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

src/utils.jl

src/R2N.jl

src/LM_alg.jl

src/LMTR_alg.jl

Co-authored-by: Copilot <[email protected]>

dpo · 2025-10-11T19:40:41Z

@arnavk23 I think what we wanted to do in #220 is add the modules LLSModels.jl and QuadraticModels.jl as dependencies, but first, we have to add missing functionality in those modules. Some of your code here can probably be reused there.

arnavk23 · 2025-10-12T01:19:22Z

@dpo I made some new changes to the pr.

…fk hk=hk σk=σk ν=ν end end return (xk, fk, hk, σk, ν, solver) end

MaxenceGollier · 2025-10-16T18:00:11Z

Hi @arnavk23, sorry I have been busy elsewhere lately.

As @dpo said, I think we should add features to QuadraticModels.jl and LLSModels.jl before we could solve #220.
Also, you commited the Manifest.toml and there are some weird changes in the diff.

The problem I have with QuadraticModels is that, if I remember correctly, it allocates and contains type instabilities.
Moreover, neither LLModels.jl nor QuadraticModels supports quadratic regularization.

Finally, we recently added a skip_sigma keyword argument in the objective of the R2NModel and we should add something similar for regularized QuadraticModels or LLSModels.

…e and solved flag from the current ξ and ν before calling set_status!.

Copilot

Pull Request Overview

Copilot reviewed 8 out of 10 changed files in this pull request and generated 10 comments.

Comments suppressed due to low confidence (1)

src/LM_alg.jl:1

This signature removes the previous sub_kwargs forwarding of extra keyword arguments to the inner subsolver. If external code relies on passing subproblem options, this is a breaking change. Consider accepting kwargs... and forwarding them in the inner solve! calls.

export LM, LMSolver, solve!

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

src/R2N.jl

Copilot · 2025-10-17T18:19:16Z

src/R2N.jl

+    x0_quad,
+    reg_hess_op,
+    reg_hess_wrapper,
  )


R2NSolver adds 4 new fields (x0_quad, reg_hess, reg_hess_wrapper, reg_hess_op) but the constructor appends only 3 values. This leaves reg_hess_op uninitialized and mis-assigns reg_hess to reg_hess_op, causing construction errors. Append a fourth argument and align ordering, e.g.:
x0_quad, reg_hess_op, reg_hess_wrapper, reg_hess_op
(or drop the redundant reg_hess field).

src/R2N.jl

Copilot · 2025-10-17T18:19:17Z

src/LM_alg.jl

    (ξ1 < 0 && sqrt_ξ1_νInv > neg_tol) &&
      error("LM: prox-gradient step should produce a decrease but ξ1 = $(ξ1)")

+    # Recompute stationarity measure and solved flag using current ξ and ν
+    sqrt_ξ1_νInv = ξ ≥ 0 ? sqrt(ξ / ν) : sqrt(-ξ / ν)
+    solved = (ξ < 0 && sqrt_ξ1_νInv ≤ neg_tol) || (ξ ≥ 0 && sqrt_ξ1_νInv ≤ atol)


ξ1 was renamed to ξ above, but these lines still reference ξ1, which is undefined. Replace ξ1 with ξ in the condition and error message.

src/LMTR_alg.jl

src/R2N.jl

Copilot · 2025-10-17T18:19:18Z

src/R2N.jl

+  return qm
+end


The function accepts H but never applies it. Either remove the unused parameter or support in-place Hessian updates:
if H !== nothing; qm.data.H = H; end.

src/LM_alg.jl

Co-authored-by: Copilot <[email protected]>

arnavk23 added 2 commits October 9, 2025 16:25

R2N

3774c3d

allocations_56240

685dabb

dpo requested review from MaxenceGollier and Copilot October 10, 2025 14:38

Copilot AI reviewed Oct 10, 2025

View reviewed changes

src/R2N.jl Show resolved Hide resolved

src/LMTR_alg.jl Show resolved Hide resolved

src/R2N.jl Outdated Show resolved Hide resolved

src/LM_alg.jl Show resolved Hide resolved

src/R2N.jl Outdated Show resolved Hide resolved

arnavk23 and others added 3 commits October 10, 2025 20:11

Update src/R2N.jl

d984a62

Co-authored-by: Copilot <[email protected]>

Update src/R2N.jl

8473946

Co-authored-by: Copilot <[email protected]>

Update src/LM_alg.jl

9f9322f

Co-authored-by: Copilot <[email protected]>

arnavk23 requested a review from Copilot October 10, 2025 20:35

Copilot AI reviewed Oct 10, 2025

View reviewed changes

arnavk23 and others added 6 commits October 11, 2025 02:54

Update src/LMTR_alg.jl

4a0d87c

Co-authored-by: Copilot <[email protected]>

Update src/LMTR_alg.jl

8ed914e

Co-authored-by: Copilot <[email protected]>

Update src/R2N.jl

b7d2979

Co-authored-by: Copilot <[email protected]>

Update src/R2N.jl

b06e017

Co-authored-by: Copilot <[email protected]>

Update src/LM_alg.jl

dd6fc3b

Co-authored-by: Copilot <[email protected]>

changes

37e9beb

arnavk23 requested a review from Copilot October 11, 2025 08:09

Copilot AI reviewed Oct 11, 2025

View reviewed changes

arnavk23 and others added 8 commits October 11, 2025 13:41

Update src/utils.jl

8357983

Co-authored-by: Copilot <[email protected]>

Update src/R2N.jl

589f75b

Co-authored-by: Copilot <[email protected]>

Update src/R2N.jl

0f9f29e

Co-authored-by: Copilot <[email protected]>

Update src/R2N.jl

13ce83c

Co-authored-by: Copilot <[email protected]>

Update src/R2N.jl

3cef1e2

Co-authored-by: Copilot <[email protected]>

Update src/LM_alg.jl

4e1e1f9

Co-authored-by: Copilot <[email protected]>

Update src/LMTR_alg.jl

7c7aeaf

Co-authored-by: Copilot <[email protected]>

changeset 1a2b3c4d5e6f7g8h9i0jklmnopqrstuv

f2f89ed

More Changes

a434d5a

arnavk23 added 2 commits October 13, 2025 01:15

g @warn "LM: subsolver iter count high" iter=solver.substats.iter fk=…

2ea55a4

…fk hk=hk σk=σk ν=ν end end return (xk, fk, hk, σk, ν, solver) end

LM_alg

e3bd03b

arnavk23 added 3 commits October 17, 2025 16:51

Changes proposed

48811bd

Merge branch 'master' into master

92c7efb

Update the in-loop status update to recompute the stationarity measur…

109c004

…e and solved flag from the current ξ and ν before calling set_status!.

arnavk23 requested a review from Copilot October 17, 2025 18:13

Copilot AI reviewed Oct 17, 2025

View reviewed changes

arnavk23 and others added 6 commits October 18, 2025 00:09

Update src/R2N.jl

b5e1c58

Co-authored-by: Copilot <[email protected]>

Update src/LMTR_alg.jl

deef93e

Co-authored-by: Copilot <[email protected]>

Update src/R2N.jl

fb0fbb3

Co-authored-by: Copilot <[email protected]>

Update src/LM_alg.jl

2d08604

Co-authored-by: Copilot <[email protected]>

Copilot remarks

8dd1b2a

solver error

f5d105e

arnavk23 closed this Oct 18, 2025

LMModel → LLSModels and R2NModel → QuadraticModels #244

LMModel → LLSModels and R2NModel → QuadraticModels #244

Uh oh!

Conversation

arnavk23 commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Technical Implementation

Uh oh!

arnavk23 commented Oct 3, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dpo commented Oct 11, 2025

Uh oh!

arnavk23 commented Oct 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaxenceGollier commented Oct 16, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

arnavk23 commented Oct 3, 2025 •

edited

Loading

arnavk23 commented Oct 12, 2025 •

edited

Loading