Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use generic sparsity detector #238

Closed

Conversation

gdalle
Copy link
Collaborator

@gdalle gdalle commented Jun 4, 2024

ADTypes.jl v1.0 introduced an abstract sparse autodiff interface for sparsity pattern detection and matrix coloring, which can be implemented by various packages. The latest release of Symbolics.jl, namely v5.29, includes a SymbolicsSparsityDetector object which satisfies the ADTypes.jl requirements of sparsity pattern detection.

My PR changes your sparsity detection routines to use this detector object, in order to make it easier to switch later on. For instance, you may want to use TracerSparsityDetector from SparseConnectivityTracer.jl instead (see #229 and #230).
The only caveat is that it uses a random vector of Lagrange multiplier, but I think that's correct since the probability of accidental cancellation between constraints is basically zero?

I also removed the compat tweak on SymbolicUtils.jl in the test environment, since it has released a v2.0 which apparently fixes #232.

Ping @amontoison

@amontoison
Copy link
Member

Thanks @gdalle!
Can I finalize the PR for SCT.jl after this one?

@gdalle
Copy link
Collaborator Author

gdalle commented Jun 4, 2024

Sure, it will be even easier to finalize then, just switch the detector object.
The only thing you have to do is figure out if you want to pass this detector all the way down the call chain, e.g. by making it part of one of your structures.

@gdalle
Copy link
Collaborator Author

gdalle commented Jun 4, 2024

I suspect the doc failure is due to the ungodly way in which you handle Symbolics. It's not in your Project.toml but it's still a dependency somehow. How does that work?
It's probably due to your well-known and unjustified hatred of package extensions, but really this is a terrible footgun.

Copy link
Contributor

github-actions bot commented Jun 4, 2024

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
Copy link
Collaborator Author

gdalle commented Jun 4, 2024

Oops, for SymbolicsSparsityDetector to exist I need the latest version of Symbolics, but they have dropped support for Julia 1.6 a long time ago. If you check the CI log on main, you'll see that you're using an old version of Symbolics.
Note that SparseConnectivityTracer.jl supports Julia 1.6 ;)

@amontoison
Copy link
Member

amontoison commented Jun 4, 2024

I propose to update the compat entry of Julia to v1.9 such we have support for package extensions.
The next release will be breaking in all cases because SCT.jl will be the default.
I must also rename the Symbolics backends.

@gdalle
Copy link
Collaborator Author

gdalle commented Jun 4, 2024

In that case it probably doesn't make much sense to try hard to support Symbolics again. Maybe you can just rebase your SCT PR on this one, switch the detector and we drop Symbolics once and for all?

@amontoison
Copy link
Member

In that case it probably doesn't make much sense to try hard to support Symbolics again. Maybe you can just rebase your SCT PR on this one, switch the detector and we drop Symbolics once and for all?

Yes, let's do that.

@gdalle
Copy link
Collaborator Author

gdalle commented Jun 4, 2024

Closing in favor of #230

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug with upgrade to SymbolicUtils release 1.6 and 1.7
2 participants