Use SparseConnectivityTracer.jl #230

amontoison · 2024-05-18T01:06:19Z

github-actions · 2024-05-18T01:15:59Z

Package name	latest	stable
CaNNOLeS.jl
DCISolver.jl
DerivativeFreeSolvers.jl
JSOSolvers.jl
NLPModelsIpopt.jl
OptimalControl.jl
OptimizationProblems.jl
Percival.jl
QuadraticModels.jl
SolverBenchmark.jl
SolverTools.jl

gdalle · 2024-05-19T13:36:51Z

Can you try it with the branch from adrhill/SparseConnectivityTracer.jl#75 ?

tmigot · 2024-05-20T14:59:23Z

Hi @amontoison @gdalle ! Thanks for adding this new feature. Can you make it that we don't remove the Symbolics-way of computing the sparse matrices? I am fine having both ways.

amontoison · 2024-05-20T15:04:42Z

@tmigot What do you want to keep Symbolics.jl?

tmigot · 2024-05-20T15:09:31Z

@tmigot What do you want to keep Symbolics.jl?

Why not? I don't think that the debate is 100% clear that the SparseConnectivityTracer is better than Symbolics, and it doesn't cost us a lot to maintain Symbolics for now.

gdalle · 2024-05-20T15:33:11Z

If you want to switch easily between both, I'd suggest using the ADTypes interface for sparsity detection. SparseConnectivityTracer supports it natively, and Symbolics will once JuliaSymbolics/Symbolics.jl#1134 is merged. Then it will be as easy as switching the "detector" object

https://sciml.github.io/ADTypes.jl/stable/#Sparsity-detector

gdalle · 2024-05-22T19:08:03Z

Wait for adrhill/SparseConnectivityTracer.jl#90

amontoison · 2024-06-04T16:03:51Z

@gdalle Do you have functions to perform J*u, J'*v, H*w in SCT.jl?
I already dropped Symbolics.jl and I can drop SparseDiffTools.jl if I have these routines.

amontoison · 2024-06-04T16:14:28Z

@gdalle It seems that this case is not supported in ADTypes: https://github.com/amontoison/ADNLPModels.jl/blob/SCT/src/sparsity_pattern.jl#L9-L13

gdalle · 2024-06-04T16:15:01Z

Do you have functions to perform J*u, J'v, Hw in SCT.jl?

What do you mean by that? The result of a matrix-vector product is dense in general, why would you want its sparsity pattern?

gdalle · 2024-06-04T16:16:13Z

@gdalle It seems that this case is not supported in ADTypes: https://github.com/amontoison/ADNLPModels.jl/blob/SCT/src/sparsity_pattern.jl#L9-L13

You need to use the function from ADTypes with the detector

function compute_jacobian_sparsity(c, x0)
  detector = SparseConnectivityTracer.TracerSparsityDetector()  # replaceable
  S = ADTypes.jacobian_sparsity(c, x0, detector)
  return S
end

amontoison · 2024-06-04T16:38:33Z

@gdalle It seems that this case is not supported in ADTypes: https://github.com/amontoison/ADNLPModels.jl/blob/SCT/src/sparsity_pattern.jl#L9-L13

You need to use the function from ADTypes with the detector
function compute_jacobian_sparsity(c, x0)
  detector = SparseConnectivityTracer.TracerSparsityDetector()  # replaceable
  S = ADTypes.jacobian_sparsity(c, x0, detector)
  return S
end

My bad, I did the typo...

gdalle · 2024-06-04T18:32:35Z

@gdalle Do you have functions to perform J*u, J'v, Hw in SCT.jl?

These functions are not connected to the sparsity pattern. The right way to perform them is with DifferentiationInterface

amontoison · 2024-06-05T00:50:58Z

@gdalle Can you review the PR?

gdalle

looks good to me!

docs/src/predefined.md

src/sparse_hessian.jl

src/sparsity_pattern.jl

test/runtests.jl

tmigot · 2024-06-06T05:39:02Z

Hi guys! Thanks @gdalle and @amontoison for this.
Why did you end up droping Symbolics in the same PR?
Have you guys run some performance benchmark?

gdalle · 2024-06-06T05:50:15Z

There are several reasons, which @amontoison summed up here: control-toolbox/CTDirect.jl#88 (comment).
A big one was the compatibility issues of Symbolics (some recent versions were buggy, and it hasn't supported Julia 1.6 for a while) and the general weight of the SciML stack of dependencies, as opposed to the very lightweight SparseConnectivityTracer.

We didn't benchmark specifically against Symbolics on optimization problems. However we did run other benchmarks:

SparseConnectivityTracer against Symbolics on the Jacobian test case used in the very article that introduced Symbolics sparsity detection: Compare with Symbolics adrhill/SparseConnectivityTracer.jl#4 (comment)
SparseConnectivityTracer against JuMP: Benchmarking against JuMP: faster on Jacobians, slower on Hessians adrhill/SparseConnectivityTracer.jl#118

Finally, note that SparseConnectivityTracer is only a default choice, not an eternal commitment. Thanks to the new ADTypes sparsity detection API, going back to Symbolics is basically as easy as replacing the detector object in the code below with SymbolicsSparsityDetector. I explained it in the PR #238 before it was superseded by the present one.

ADNLPModels.jl/src/sparsity_pattern.jl

Lines 9 to 16 in 29b1d2d

    
           function compute_jacobian_sparsity( 
        
             c, 
        
             x0; 
        
             detector::AbstractSparsityDetector = TracerSparsityDetector(), 
        
           ) 
        
             S = ADTypes.jacobian_sparsity(c, x0, detector) 
        
             return S 
        
           end

I don't think this detector configuration is accessible to the user yet, because it is still in the low-level functions, but it would be good to pass this object down all the way from the top level API

tmigot · 2024-06-06T06:05:38Z

There are several reasons, which @amontoison summed up here: control-toolbox/CTDirect.jl#88 (comment). A big one was the compatibility issues of Symbolics (some recent versions were buggy, and it hasn't supported Julia 1.6 for a while)

Ok, but it is not a real constraint, it was just a versioning mistake that is being resolved.

and the general weight of the SciML stack of dependencies, as opposed to the very lightweight SparseConnectivityTracer.

Agreed.

I am in general very in favor of what you guys did here. My point was mainly that it is strange to do everything in the same PR especially if we don't benchmark this package. The correct strategy should have been:

add SparseConnectivityTracer
add a benchmark SparseConnectivityTracer/Symbolics
discuss about droping Symbolics ? (also it was only an optional dep so it didn't slow down the user experience, only the unit tests)

Could you open an issue to remember us to make the detector configuration accessible to users? Thanks!

gdalle · 2024-06-06T06:15:35Z

I agree with the temporary aspect of the SymbolicUtils kerfuffle. However, Julia 1.6 support was dropped some time ago by Symbolics and it isn't coming back. So that's a real limitation if you want to continue supporting 1.6, at least until 1.10 is declared as the new LTS.

I agree that big changes all at once are not ideal.
Initially I tried a soft transition in PR #238 (just using the detector framework, but sticking with Symbolics). I got the tests working on 1.10 but they failed on 1.6 because of the missing SymbolicsSparsityDetector object (which was introduced after 1.6 support was dropped from Symbolics). I guess the SymbolicsSparsityDetector could have been backported to a version of Symbolics that does support 1.6, but given the tiresomeness of it all, Alexis and I thought it would be best to do a clean break

gdalle · 2024-06-06T06:21:04Z

I opened an issue to keep track of the detector configuration, and I'll add benchmarks against Symbolics to my SCT/JuMP comparison in adrhill/SparseConnectivityTracer.jl#118

amontoison · 2024-06-06T07:36:10Z

There are several reasons, which @amontoison summed up here: control-toolbox/CTDirect.jl#88 (comment). A big one was the compatibility issues of Symbolics (some recent versions were buggy, and it hasn't supported Julia 1.6 for a while)

Ok, but it is not a real constraint, it was just a versioning mistake that is being resolved.

I wanted to have sparse AD by default here.
The AD was sparse only if Symbolics.jl was installed.
Note thay Symbolics.jl downloads all the internet when we install it.
Because it was an optional dependency, we were unable to set a compat entry on it and depending on multiple factors, the version installed could be incompatible or broken....
It's not something that I want for the users of ADNLPModels.jl.

and the general weight of the SciML stack of dependencies, as opposed to the very lightweight SparseConnectivityTracer.

Agreed.

I am in general very in favor of what you guys did here. My point was mainly that it is strange to do everything in the same PR especially if we don't benchmark this package. The correct strategy should have been:

add SparseConnectivityTracer

add a benchmark SparseConnectivityTracer/Symbolics

discuss about droping Symbolics ? (also it was only an optional dep so it didn't slow down the user experience, only the unit tests)

I wanted to drop Symbolics.jl and SparsityPatternDetection.jl and it was the best strategy for me.
Even if we lose performances (in the worst case), we can improve things with Guillaume and Adrian.
We opened issues in Symbolics.jl two years ago and nothing was solved...

Could you open an issue to remember us to make the detector configuration accessible to users? Thanks!

It's already available as a keyword argument for the sparse AD backends (SparseADHessian and SparseADJacobian).

amontoison marked this pull request as draft May 18, 2024 01:10

This was referenced May 19, 2024

Add optimization problems to "real-world" tests adrhill/SparseConnectivityTracer.jl#69

Closed

Problems with Symbolics? control-toolbox/CTDirect.jl#88

Closed

gdalle mentioned this pull request May 24, 2024

Upgrade to ColPack v0.4 #235

Merged

amontoison force-pushed the SCT branch from 3beaae8 to 703f818 Compare May 28, 2024 15:24

gdalle added 2 commits June 4, 2024 08:22

Use generic sparsity detector

9af0a7e

Working

873e150

gdalle mentioned this pull request Jun 4, 2024

Use generic sparsity detector #238

Closed

gdalle and others added 5 commits June 4, 2024 08:38

Versions

50e4e2c

Up Symbolics in docs

1d7ead4

Use SparseConnectivityTracer.jl

4616cdd

Modifications done during the flight

c832143

Update SCT.jl

2425a65

amontoison force-pushed the SCT branch from 703f818 to 2425a65 Compare June 4, 2024 15:54

amontoison marked this pull request as ready for review June 4, 2024 15:56

Remove Symbolics.jl

5d99aaa

Keep CUDA.jl v4 for the tests...

370717a

amontoison added 2 commits June 4, 2024 19:26

Update the documentation and fix a few typos

9c4b9ef

Update runtests.jl

d1cde67

amontoison requested a review from tmigot June 5, 2024 00:38

amontoison mentioned this pull request Jun 5, 2024

Full support of sparse AD for NLS models #239

Merged

gdalle approved these changes Jun 5, 2024

View reviewed changes

Add an option to provide a detector

9529aa2

amontoison merged commit b5b61f9 into JuliaSmoothOptimizers:main Jun 5, 2024
32 of 33 checks passed

amontoison deleted the SCT branch June 5, 2024 15:40

gdalle mentioned this pull request Jun 6, 2024

Let users configure sparsity detector and coloring algorithm #242

Closed

gdalle mentioned this pull request Jun 11, 2024

Replace ColPack with SparseMatrixColorings #244

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use SparseConnectivityTracer.jl #230

Use SparseConnectivityTracer.jl #230

amontoison commented May 18, 2024 •

edited

Loading

github-actions bot commented May 18, 2024 •

edited

Loading

gdalle commented May 19, 2024

tmigot commented May 20, 2024

amontoison commented May 20, 2024

tmigot commented May 20, 2024

gdalle commented May 20, 2024 •

edited

Loading

gdalle commented May 22, 2024

amontoison commented Jun 4, 2024 •

edited

Loading

amontoison commented Jun 4, 2024

gdalle commented Jun 4, 2024

gdalle commented Jun 4, 2024

amontoison commented Jun 4, 2024

gdalle commented Jun 4, 2024

amontoison commented Jun 5, 2024

gdalle left a comment

tmigot commented Jun 6, 2024

gdalle commented Jun 6, 2024 •

edited

Loading

tmigot commented Jun 6, 2024

gdalle commented Jun 6, 2024 •

edited

Loading

gdalle commented Jun 6, 2024

amontoison commented Jun 6, 2024 •

edited

Loading

Use SparseConnectivityTracer.jl #230

Use SparseConnectivityTracer.jl #230

Conversation

amontoison commented May 18, 2024 • edited Loading

github-actions bot commented May 18, 2024 • edited Loading

gdalle commented May 19, 2024

tmigot commented May 20, 2024

amontoison commented May 20, 2024

tmigot commented May 20, 2024

gdalle commented May 20, 2024 • edited Loading

gdalle commented May 22, 2024

amontoison commented Jun 4, 2024 • edited Loading

amontoison commented Jun 4, 2024

gdalle commented Jun 4, 2024

gdalle commented Jun 4, 2024

amontoison commented Jun 4, 2024

gdalle commented Jun 4, 2024

amontoison commented Jun 5, 2024

gdalle left a comment

Choose a reason for hiding this comment

tmigot commented Jun 6, 2024

gdalle commented Jun 6, 2024 • edited Loading

tmigot commented Jun 6, 2024

gdalle commented Jun 6, 2024 • edited Loading

gdalle commented Jun 6, 2024

amontoison commented Jun 6, 2024 • edited Loading

amontoison commented May 18, 2024 •

edited

Loading

github-actions bot commented May 18, 2024 •

edited

Loading

gdalle commented May 20, 2024 •

edited

Loading

amontoison commented Jun 4, 2024 •

edited

Loading

gdalle commented Jun 6, 2024 •

edited

Loading

gdalle commented Jun 6, 2024 •

edited

Loading

amontoison commented Jun 6, 2024 •

edited

Loading