Skip to content

Add biadjacency matrix helpers#1589

Open
spital wants to merge 2 commits into
Qiskit:mainfrom
spital:issue-1412-biadjacency
Open

Add biadjacency matrix helpers#1589
spital wants to merge 2 commits into
Qiskit:mainfrom
spital:issue-1412-biadjacency

Conversation

@spital

@spital spital commented May 27, 2026

Copy link
Copy Markdown
Contributor

Fixes #1412

Summary

This adds dense NumPy biadjacency matrix support for both graph types:

  • PyGraph.from_biadjacency_matrix()
  • PyDiGraph.from_biadjacency_matrix()
  • rustworkx.graph_biadjacency_matrix()
  • rustworkx.digraph_biadjacency_matrix()

For an input matrix with shape (m, n), the constructors create m + n nodes, with row nodes first and column nodes second. Non-null entries create edges between the corresponding row an
d column nodes; for PyDiGraph, those edges are directed from row nodes to column nodes.

The output functions take explicit row and column node-index orders. These orders must contain existing node indices, must not contain duplicates, and must be disjoint. Parallel edges supp
ort the same sum, min, max, and avg reductions as the existing adjacency matrix helpers.

Why

Issue #1412 requested NetworkX-like biadjacency matrix functionality for non-square bipartite matrix workflows. This fills that API gap while keeping the behavior explicit for rustworkx no
de-index based graphs.

Notes for reviewers

  • Added tests for construction, non-zero and NaN null values, dtype validation, empty dimensions, directed edge directionality, undirected reverse-edge handling, parallel-edge reductions
    , invalid parallel_edge, missing/removed node indices, duplicate orders, overlapping orders, and empty output orders.
  • Added type stubs, API docs entries, and a release note.
  • Includes a small clippy-only cleanup in rustworkx-core/src/planar/lr_planar.rs so current cargo clippy -D warnings passes.

Checks run

  • git diff --check
  • cargo fmt --all -- --check
  • cargo check --workspace --all-features
  • cargo clippy --workspace --all-targets --all-features -- -D warnings
  • nox -e lint
  • nox -e stubs
  • nox -e test -- adjacency_matrix — 85 passed
  • nox -e test — 2410 run, 2399 passed, 11 skipped

@CLAassistant

CLAassistant commented May 27, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@coveralls

Copy link
Copy Markdown

Coverage Report for CI Build 26497143214

Warning

Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes.
Quick fix: rebase this PR. Learn more →

Coverage increased (+0.06%) to 94.784%

Details

  • Coverage increased (+0.06%) from the base build.
  • Patch coverage: 241 of 241 lines across 5 files are fully covered (100%).
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 20378
Covered Lines: 19315
Line Coverage: 94.78%
Coverage Strength: 917053.97 hits per line

💛 - Coveralls

@IvanIsCoding IvanIsCoding left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

So there are many comments. On the most immediate part, look at https://github.com/Qiskit/rustworkx/blob/main/rustworkx/__init__.py for a reference on the universal functions. And I left a couple Rust comments.

With that being said, the biggest flaw in the PR is that we don't return or accept any types from https://docs.scipy.org/doc/scipy/reference/sparse.html. This is what users expect when dealing with sparse arrays.

You'll need to rewrite your whole code to not use NumPy and tweak tests such that scipy does not become a required dependency. It should be an optional dependency.

One way of decoupling our code from scipy is to leverage #1515. We can serialize to matrix market and then deserialize from it with scipy for rustworkx.biadjacency_matrix. And the other way around for from_biadjacency_matrix, ask scipy to export a format we can read and then load it.

Comment thread rustworkx-core/src/planar/lr_planar.rs Outdated
self.lowpt.insert(ei, self.height[&w]);
self.lowpt_2.insert(ei, self.height[&v]);
}
// do *not* consider ``(v, w)`` as a back edge if ``(w, v)`` is a tree edge.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Remove this diff it's unrelated to the PR

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you for comments @IvanIsCoding , looking to address them.
This particular change was due to cargo clippy --workspace --all-targets --all-features -- -D warnings failing for cargo 1.95; Rust 1.96 released recently.
I understand CI might pin lower version, but maybe useful for the future, would you find useful if I create new PR for this little change to fix future clippy ?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Sure, feel free to bump the Rust versions for Clippy and the formatter. Just try to keep it separate from this PR

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in #1595

Comment on lines +6 to +7
:func:`~rustworkx.graph_biadjacency_matrix`, and
:func:`~rustworkx.digraph_biadjacency_matrix` for dense NumPy

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

So document the individual methods for the PyDiGraph and PyGraph methods (e.g. from_biadjacency_matrix have different documentation entries). But for biadjacency_matrix, you should only note it once in the release with the universal function you are missing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated. The note now documents PyGraph.from_biadjacency_matrix and PyDiGraph.from_biadjacency_matrix individually

Comment thread rustworkx/__init__.pyi
from .rustworkx import weakly_connected_components as weakly_connected_components
from .rustworkx import digraph_adjacency_matrix as digraph_adjacency_matrix
from .rustworkx import graph_adjacency_matrix as graph_adjacency_matrix
from .rustworkx import digraph_biadjacency_matrix as digraph_biadjacency_matrix

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A reminder to:

  1. Add a universal function with @_rustworkx_dispatch that works with both graph types
  2. Document that function stub here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added. rustworkx.biadjacency_matrix now lives in rustworkx/__init__.py decorated with @_rustworkx_dispatch

Comment thread src/connectivity/mod.rs Outdated
parallel_edge_fn: ParallelEdgeFn,
) {
let key = [row, column];
let count = parallel_edge_count.entry(key).or_insert(0);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Use a match with the entry() method:

Entry::Occupied(mut entry) => {

It will save one hashmap access. Those add up. The code is also cleaner.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, the parallel-edge accumulator now matches on HashMap::entry() with Vacant / Occupied arms

Comment thread src/connectivity/mod.rs Outdated
Ok(matrix.into_pyarray(py))
}

type ParallelEdgeFn = fn(f64, f64, usize) -> f64;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

For code organization:

  1. create a biadjacency.rs in this module and import it
  2. move the duplicate code from digraph.rs and graph.rs here with generics as well

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, added src/connectivity/biadjacency.rs

Comment thread src/connectivity/mod.rs
text_signature = "(graph, row_order, column_order, /, weight_fn=None, default_weight=1.0, null_value=0.0, parallel_edge=\"sum\")"
)]
#[allow(clippy::too_many_arguments)]
pub fn digraph_biadjacency_matrix<'py>(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

After you create the other file, keep the main function here still

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment thread src/connectivity/mod.rs Outdated
let parallel_edge_fn = parallel_edge_fn(parallel_edge)?;
let row_map = biadjacency_node_map(&row_order);
let column_map = biadjacency_node_map(&column_order);
let mut matrix = Array2::<f64>::from_elem((row_order.len(), column_order.len()), null_value);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

So this is the fatal flaw in the PR: this shouldn't be a NumPy array. This should be a scipy object, one of their sparse arrays.

You'll need to follow https://docs.rs/pyo3/0.26.0/pyo3/#example-using-python-from-rust to interact with Python objects from SciPy. Import the module, throw an error if it is not there, and then proceed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reworked. The export no longer produces a NumPy array.
It imports scipy.sparse through PyO3, builds a coo_array from the triplets and returns .asformat(format) with a new format parameter defaulting to "csr". null_value is gone

Comment thread src/digraph.rs Outdated

fn _from_biadjacency_matrix<'p, T>(
py: Python<'p>,
matrix: PyReadonlyArray2<'p, T>,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same flaw, deal with scipy objects. You can check how NetworkX handles it for a reference.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same treatment on the input side

)


class TestDiGraphBiadjacencyMatrix(unittest.TestCase):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As a reminder, tests will look different. #1221 has the old code we deleted but the summary is: tests are optional, if scipy is not present tests should not run. We should update pyproject.toml to only install scipy for a range of python versions and preferably skip Windows as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated



class TestGraphBiadjacencyMatrix(unittest.TestCase):
def test_from_biadjacency_matrix(self):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same comment about test setup

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same as the digraph side.
skipIf guard plus the pyproject.toml marker above

spital added 2 commits June 21, 2026 13:49
Add constructors and matrix export functions for dense NumPy
biadjacency matrices on PyGraph and PyDiGraph. The constructors create
row nodes before column nodes, and the export functions use explicit
row and column node-index orders.

Also add type stubs, API documentation, a release note, and tests for
construction, null values, validation, directed edge direction, and
parallel-edge reductions.

Fixes Qiskit#1412
Address review feedback on Qiskit#1589:

* Return scipy sparse arrays
* Add the universal rustworkx.biadjacency_matrix function
* Move the shared conversion code from graph.rs/digraph.rs/connectivity
  into src/connectivity/biadjacency.rs using generics
* Use a match on HashMap::entry() when accumulating parallel edges to save a hashmap access
* Make biadjacency tests skip when SciPy is not installed and add scipy
  to the test extra only for CPython >= 3.11 on non-Windows platforms.
* Drop the unrelated lr_planar.rs clippy change (split out to Qiskit#1595).
@spital spital force-pushed the issue-1412-biadjacency branch from 4432858 to e1240a1 Compare June 22, 2026 12:49
@spital

spital commented Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review.
I rewrote the matrix side to use SciPy sparse arrays instead of dense NumPy, and addressed every inline comment:

  • SciPy sparse in/out, SciPy optional. biadjacency_matrix builds a scipy.sparse.coo_array and returns .asformat(format) (default "csr"); from_biadjacency_matrix accepts any SciPy sparse matrix/array via .tocoo(). SciPy is imported at call time and raises a clear ImportError if it's missing, it is not a required runtime dependency.
  • Universal dispatcher. Added rustworkx.biadjacency_matrix with @_rustworkx_dispatch plus its stub.
  • Code organization. Shared logic moved to src/connectivity/biadjacency.rs with a generic stable_graph_from_biadjacency_matrix<Ty: EdgeType>; the public #[pyfunction]s stay in connectivity/mod.rs. Parallel-edge accumulation now uses match on entry().
  • Tests optional. Biadjacency tests skipIf SciPy is absent; pyproject.toml installs SciPy only for python_version >= '3.11' and platform_system != 'Windows'.
  • Rebased onto current main (0.18.0). The diff is now purely additive and no longer touches lr_planar.rs (that bump is in Bump **CI** Clippy and rustfmt toolchain to Rust 1.96 #1595).

Two small design notes I'd appreciate your take on:

  1. I kept a parallel_edge option ("sum" default, plus min/max/avg) for consistency with the existing adjacency_matrix. Happy to drop it if you'd rather mirror NetworkX exactly.
  2. graph_/digraph_biadjacency_matrix currently require row_order and column_order to be disjoint (a clear ValueError otherwise). That's natural for bipartite use, but it's stricter than NetworkX. Let me know if you'd prefer to allow overlap (e.g. a directed square sub-matrix).

@spital spital requested a review from IvanIsCoding June 23, 2026 18:15
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.

Add networkx's from_biadjacency_matrix and biadjacency_matrix

4 participants