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

Allow more generic hessian set types #103

Merged
merged 12 commits into from
May 31, 2024
Merged

Allow more generic hessian set types #103

merged 12 commits into from
May 31, 2024

Conversation

gdalle
Copy link
Collaborator

@gdalle gdalle commented May 24, 2024

This PR is not done yet

Source

  • Add two variants of connectivity_tracer_i_to_j, gradient_tracer_i_to_j and hessian_tracer_i_to_j: one for AbstractTracer and one for AbstractSet
  • Use gradient_tracer_i_to_j inside hessian_tracer_i_to_j for separation of concerns
  • Allow more generic eltype in the various custom sets, so that they can hold Tuple{Int,Int}
  • Start implementing clever union! and product, but we will need the more sophisticated union_product! I introduced for Hessians

Tests

  • Add some tests for cartesian products
  • Start adding tests for Hessian tracers with H != Set{Tuple{Int,Int}}

@gdalle gdalle linked an issue May 24, 2024 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented May 24, 2024

Codecov Report

Attention: Patch coverage is 89.86486% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 84.19%. Comparing base (59ae75c) to head (2b64b8c).
Report is 1 commits behind head on main.

Files Patch % Lines
src/settypes/recursiveset.jl 33.33% 8 Missing ⚠️
src/overload_connectivity.jl 87.50% 3 Missing ⚠️
src/settypes/duplicatevector.jl 80.00% 2 Missing ⚠️
src/overload_hessian.jl 97.22% 1 Missing ⚠️
src/tracers.jl 93.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #103      +/-   ##
==========================================
- Coverage   84.56%   84.19%   -0.37%     
==========================================
  Files          18       18              
  Lines         758      829      +71     
==========================================
+ Hits          641      698      +57     
- Misses        117      131      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gdalle
Copy link
Collaborator Author

gdalle commented May 30, 2024

@adrhill can you take a look and validate the general direction?

Copy link
Owner

@adrhill adrhill left a comment

Choose a reason for hiding this comment

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

The new overloads are exactly what I imagined. 👍

src/settypes/recursiveset.jl Outdated Show resolved Hide resolved
src/settypes/sortedvector.jl Outdated Show resolved Hide resolved
test/settypes/bitset.jl Outdated Show resolved Hide resolved
test/test_hessian.jl Outdated Show resolved Hide resolved
@gdalle gdalle marked this pull request as ready for review May 31, 2024 07:28
@gdalle gdalle added the run benchmark Run benchmarks in CI label May 31, 2024
@gdalle
Copy link
Collaborator Author

gdalle commented May 31, 2024

The benchmark failures are caused by UInt64, please merge #109 first

@adrhill
Copy link
Owner

adrhill commented May 31, 2024

#109 has been merged.

src/overload_hessian.jl Show resolved Hide resolved
src/settypes/recursiveset.jl Show resolved Hide resolved
src/tracers.jl Show resolved Hide resolved
src/settypes/duplicatevector.jl Show resolved Hide resolved
Copy link
Owner

@adrhill adrhill left a comment

Choose a reason for hiding this comment

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

Internal representations for Gradient and Hessian information should be decoupled and abstracted away. The only interface between the two should be union_product!.

This requires

  • removing type restrictions in overload_hessian.jl (see here)
  • adding tests on different Gradient and Hessian set types (see here)
  • most likely implementing more methods for union_product!

@adrhill adrhill merged commit ee6c15d into main May 31, 2024
4 of 5 checks passed
@adrhill adrhill deleted the gd/settypes branch May 31, 2024 15:46
@adrhill adrhill added patterns Features regarding sparsity pattern representation and removed run benchmark Run benchmarks in CI labels Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patterns Features regarding sparsity pattern representation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow more Hessian set types than just Set{Tuple{Int,Int}}
3 participants