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

Add SparseDiffTools jacobian #105

Merged
merged 7 commits into from
Mar 15, 2023
Merged

Add SparseDiffTools jacobian #105

merged 7 commits into from
Mar 15, 2023

Conversation

tmigot
Copy link
Member

@tmigot tmigot commented Mar 9, 2023

#13

@codecov
Copy link

codecov bot commented Mar 9, 2023

Codecov Report

Patch coverage: 96.15% and project coverage change: -0.07 ⚠️

Comparison is base (f805c74) 98.50% compared to head (b53875f) 98.44%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #105      +/-   ##
==========================================
- Coverage   98.50%   98.44%   -0.07%     
==========================================
  Files           8        8              
  Lines         938      964      +26     
==========================================
+ Hits          924      949      +25     
- Misses         14       15       +1     
Impacted Files Coverage Δ
src/ADNLPModels.jl 97.05% <0.00%> (-2.95%) ⬇️
src/ad.jl 97.46% <ø> (ø)
src/sparse_derivatives.jl 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tmigot tmigot requested a review from amontoison March 10, 2023 00:01
@tmigot
Copy link
Member Author

tmigot commented Mar 10, 2023

@amontoison I am finally ready with this! I think it will not be hard to do the same for the Hessian, later.

@dpo
Copy link
Member

dpo commented Mar 10, 2023

Awesome!!! Would you mind adding a couple of unit tests? They serve as documentation too.

Copy link
Member

@amontoison amontoison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tangi, why do you call this backend SparseForwardADJacobian?
We can decide if we want to determine the Jacobian by rows or columns.

matrix_colors(A,alg; partition_by_rows)

@tmigot
Copy link
Member Author

tmigot commented Mar 13, 2023

Tangi, why do you call this backend SparseForwardADJacobian? We can decide if we want to determine the Jacobian by rows or columns.

matrix_colors(A,alg; partition_by_rows)

Because it uses a forward-mode autodiff to compute the values.

That's right, I can add these options in the backend constructor, thanks for the suggestion.

@tmigot
Copy link
Member Author

tmigot commented Mar 14, 2023

Hey @amontoison ! I added the algorithm in kwargs and tests. However, no clue how to make the partition_by_rows work, so I created an issue in SparseDiffTools. So, I guess it is for later :).

Copy link
Member

@amontoison amontoison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Tangi!
I have a few comments but it looks good to me.
I will open a PR for sparse Hessian after this one.

Comment on lines +2 to +3
(ADNLPModels.SparseForwardADJacobian, Dict(:alg => SparseDiffTools.GreedyD1Color())),
(ADNLPModels.SparseForwardADJacobian, Dict(:alg => SparseDiffTools.AcyclicColoring())),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also have other algorithms if we want:

  • GreedyD1Color ;
  • BacktrackingColor;
  • ContractionColor;
  • GreedyStar1Color;
  • GreedyStar2Color;
  • AcyclicColoring.

I think that we can easily use the color function in the future that I interfaced last week in CUDA.jl.
I will open an issue in SparseDiffTools when it will be merged.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not all of them work, though^^. We could add this when documenting this feature maybe !?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's do that!

src/sparse_derivatives.jl Outdated Show resolved Hide resolved
src/sparse_derivatives.jl Outdated Show resolved Hide resolved
src/sparse_derivatives.jl Outdated Show resolved Hide resolved
src/sparse_derivatives.jl Outdated Show resolved Hide resolved
src/sparse_derivatives.jl Show resolved Hide resolved
src/sparse_derivatives.jl Outdated Show resolved Hide resolved
src/sparse_derivatives.jl Outdated Show resolved Hide resolved
Co-authored-by: Alexis Montoison <35051714+amontoison@users.noreply.github.com>
@github-actions
Copy link
Contributor

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

@tmigot
Copy link
Member Author

tmigot commented Mar 15, 2023

Thanks @amontoison for the improvements!

@tmigot tmigot merged commit e6ce34f into main Mar 15, 2023
@tmigot tmigot deleted the sparse-jc branch March 15, 2023 17:59
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.

None yet

3 participants