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 93.26241% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 99.11%. Comparing base (b53dbf9) to head (a0176ac).

Files with missing lines	Patch %	Lines
src/postprocessing.jl	92.85%	19 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##              main     #267      +/-   ##
===========================================
- Coverage   100.00%   99.11%   -0.89%     
===========================================
  Files           17       18       +1     
  Lines         1962     2156     +194     
===========================================
+ Hits          1962     2137     +175     
- Misses           0       19      +19     

☔ 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]

I think we should only use neutralized_first in the context of bicoloring for now. In the general symmetric case, the ranking of i with respect to j has no real meaning, and while I know that we have been using max(i, j) as a default, maybe it is time to change it? My suggestion: since vertices_in_order sorts vertices from hardest to easiest, we probably want to set color_used to true on the hardest ones first, so we could compare the rank of i and j in vertices_in_order to decide? That would also be a decent heuristic in the bicoloring case whenever we want to minimize the sum ncolors_row + ncolors_col, in my opinion.

[amontoison]

I think we should only use neutralized_first in the context of bicoloring for now.

I agree 👍

In the general symmetric case, the ranking of i with respect to j has no real meaning, and while I know that we have been using max(i, j) as a default, maybe it is time to change it?

Yes, I also agree.
Maybe we can find a parameter similar to neutralized_first to specify which quantity we want to minimize during post-processing:

• Total number of colors (default)
• number of row colors (only for bicoloring)
• number of column colors (only for bicoloring)

My suggestion: since vertices_in_order sorts vertices from hardest to easiest, we probably want to set color_used to true on the hardest ones first, so we could compare the rank of i and j in vertices_in_order to decide? That would also be a decent heuristic in the bicoloring case whenever we want to minimize the sum ncolors_row + ncolors_col, in my opinion.

I don't think that using vertices_in_order is really relevant here because it was computed for the "full initial graph" on which we do the (bi)coloring and not for the "reduced graph" that only contains the trivial stars with unknown hubs.
It is still a valid heuristic but we can be smarter.

I think I have the optimal solution for minimizing the sum ncolors_row + ncolors_col (in the post-processing).
I will try to explain it in #264 and justify why it is optimal.

[gdalle]

Maybe we can find a parameter similar to neutralized_first to specify which quantity we want to minimize during post-processing

Something like postprocessing_minimizes = :all_colors / :row_colors / :column_colors? With the assumption that the last two settings fall back on :all_colors whenever we're in the unidirectional case.
With that parameter, we don't need neutralize_first.

[amontoison]

LGTM, but how do we specify that is a unidirectional case? Shoud we add a boolean argument?

[gdalle]

Yeah, we can do that.
When thinking about the natural order variants for bicoloring I found myself forced to pass around the original shape of A (in addition to the adjacency graph of the possibly augmented A), so that's another option

[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.