Revisit the post-processing

[amontoison]

Related to #264
Guillaume, it is still a draft but you can already have a look.

[codecov]

Codecov Report

❌ Patch coverage is 96.89922% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 99.80%. Comparing base (2cf5f8b) to head (6b0fbb1).

Files with missing lines	Patch %	Lines
src/postprocessing.jl	96.52%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##              main     #267      +/-   ##
===========================================
- Coverage   100.00%   99.80%   -0.20%     
===========================================
  Files           20       20              
  Lines         2005     2102      +97     
===========================================
+ Hits          2005     2098      +93     
- Misses           0        4       +4     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[gdalle]

Also, how does this all generalize to the acyclic bicoloring?

[amontoison]

Also, how does this all generalize to the acyclic bicoloring?

It is exactly the same in terms of handling trivial edges.
The only difference is that we need to check the non-leaf nodes in non-trivial trees and mark them as needed for decompression, but this process is fully deterministic, just like checking hubs in non-trivial stars.

It was just late yesterday, and I didn’t find the energy to finalize the PR.
I will continue it tonight.

[amontoison]

@gdalle I have an issue in my new function postprocess_star_bicoloring, I need to check if the hub h is in the column partition (1 <= h <= n) or the row partition (n+1 <= h <= n+m) but I didn't find a way to obtain n or m from the arguments.
Because we pass the AdjacencyGraph of the augmented matrix, we can only recover n+m.

[gdalle]

We could store this information in the adjacency graph, to know whether it comes from a symmetric matrix or not, and in the latter case have access to the row/column split

[gdalle]

Or we pass an additional argument to the coloring

[amontoison]

We could store this information in the adjacency graph, to know whether it comes from a symmetric matrix or not, and in the latter case have access to the row/column split

I prefer this approach because we can know that we have "augmented adjacency matrix" this way, and thus know that we are in bicoloring context for the function postprocess!.

I will not need to pass the boolean parameter bicoloring everywhere.

[gdalle]

I suggest replacing AdjacencyGraph with:

struct AdjacencyGraph{T<:Integer,bicoloring}
    S::SparsityPatternCSC{T}
    edge_to_index::Vector{T}
    original_size::Tuple{Int,Int}
end

We no longer need has_diagonal because we have bicoloring => !has_diagonal

[amontoison]

I suggest replacing AdjacencyGraph with:

struct AdjacencyGraph{T<:Integer,bicoloring}
    S::SparsityPatternCSC{T}
    edge_to_index::Vector{T}
    original_size::Tuple{Int,Int}
end

Ok for me 👍

We no longer need has_diagonal because we have bicoloring => !has_diagonal

Yes but we don't have !has_diagonal => bicoloring. Is it fine for you that the user can't specify anymore has_diagonal = false in a non-bicoloring case?

I think adding a field bicoloring::Bool in AdjacencyGraph can handle more corner cases.

[gdalle]

Yes but we don't have !has_diagonal => bicoloring. Is it fine for you that the user can't specify anymore has_diagonal = false in a non-bicoloring case?

has_diagonal was never part of our public API to begin with, so the user never gets to specify it. I think the cases where someone wants to color a Hessian whose entire diagonal is zero are sufficiently rare for us to ignore them.
In addition, they would lead to type instabilities because they require a check on the values of the matrix before creating the AdjacencyGraph object.

[amontoison]

has_diagonal was never part of our public API to begin with, so the user never gets to specify it. I think the cases where someone wants to color a Hessian whose entire diagonal is zero are sufficiently rare for us to ignore them.

Fair point 👍
I updated the PR.

[gdalle]

Aside from my remarks above, do you have any ideas for test cases where our heuristics actually produce the result we want?

[amontoison]

Aside from my remarks above, do you have any ideas for test cases where our heuristics actually produce the result we want?

@gdalle I added the heuristic that I quickly described on Messenger in the following commit: 5fa3f7a
I only need to fuse the post-processing functions together and it should be good.

[amontoison]

@gdalle It is finally ready for a review!!