Skip to content

Clearly separate graphs from the sparsity pattern#123

Merged
amontoison merged 4 commits intomainfrom
gd/bettergraph
Sep 27, 2024
Merged

Clearly separate graphs from the sparsity pattern#123
amontoison merged 4 commits intomainfrom
gd/bettergraph

Conversation

@gdalle
Copy link
Copy Markdown
Member

@gdalle gdalle commented Sep 27, 2024

Idea

Create two clearly distinct graph structures (AdjacencyGraph and BipartiteGraph) and distinguish them from the container for the sparsity pattern.

Benefits

Now:

  • Get rid of the weird Graph{true} / Graph{false} which wasn't even a graph in the rectangular case.
  • Allow storing more stuff in the graph structure if we want (typically the number of edges as you suggested).

In the future:

  • Gain flexibility to implement graphs based on different pattern types (for other structured matrices like banded), just by making the pattern fields parametric.

Changes

  • Rename Graph into SparsePatternCSC, which is exactly a SparseMatrixCSC without nzval. Remove definitions of neighbors and friends, keep only linear algebra stuff there.
  • Create a proper AdjacencyGraph based on one SparsePatternCSC, put the neighbors logic there. It is the pendant to BipartiteGraph.
  • Count edges properly in the AdjacencyGraph (divide by two to account for symmetry).
  • Remove the constructors adjacency_graph and bipartite_graph.

@gdalle gdalle requested a review from amontoison September 27, 2024 07:45
@codecov
Copy link
Copy Markdown

codecov bot commented Sep 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (b274008) to head (92546d0).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #123   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines          829       829           
=========================================
  Hits           829       829           

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

@gdalle gdalle marked this pull request as ready for review September 27, 2024 08:19
This was referenced Sep 27, 2024
Copy link
Copy Markdown
Collaborator

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

@gdalle
Excellent PR! I really like this new structure.
I was not a big fan of the Graph with the attribute loops.
Aside from a small comment about the number of edges for AdjacencyGraph, the code looks good to me.
Could I request the name SparsityPatternCSC instead of SparsePatternCSC?
It’s even more explicit from my point of view.

Excellent job!!!

@amontoison amontoison merged commit 580cc45 into main Sep 27, 2024
@amontoison amontoison deleted the gd/bettergraph branch September 27, 2024 20:27
@amontoison
Copy link
Copy Markdown
Collaborator

It should be easily to remove the allocation of S' here now.

@gdalle
Copy link
Copy Markdown
Member Author

gdalle commented Sep 27, 2024

Indeed that was the next step of the plan, I have opened #126 to keep track

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.

2 participants