Remove ldpc dependency in deltakit-explore#309
Conversation
…on with Galois implementation
…lize pivot_rows over finite field by checking != 0 instead of GF2 specific 1.
|
|
|
|
nelimee
left a comment
nelimee
left a comment
There was a problem hiding this comment.
I think this is correct, but I think it needs more eyes on the maths and the tests just in case.
|
Anything else I can do to move things along just let me know @nelimee ^^ |
Options:
>>> from importlib.metadata import Distribution
>>> list(Distribution.discover(name="stim"))
[]
>>> list(Distribution.discover(name="deltakit_stim"))
[<importlib.metadata.PathDistribution ...>]
>>> import deltakit_stim as stim
>>> dir(stim)
['Circuit', 'CircuitErrorLocation', 'CircuitErrorLocationStackFrame', 'CircuitInstruction', 'CircuitRepeatBlock', ....]I think the right fix is to query the installed package name directly while continuing to use import deltakit_stim as stim in the test file. |
There was a problem hiding this comment.
Pull request overview
This PR migrates CSS logical-operator construction in deltakit_explorer.codes._logicals from ldpc.mod2 to galois-based GF(2) linear algebra, enabling removal of the ldpc runtime dependency from deltakit-explorer.
Changes:
- Replaced ldpc-based nullspace/rank/pivot-row logic with
galois.GF2operations and a newpivot_rows()helper in_logicals.py. - Expanded logical-construction unit tests to validate GF(2) invariants (kernel/image/basis properties and redundancy invariance) rather than a specific basis ordering.
- Dropped
ldpcfromdeltakit-explorerdependencies.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| deltakit-explorer/src/deltakit_explorer/codes/_logicals.py | Reimplements CSS logical computation using galois GF(2) operations and a new pivot-row helper. |
| deltakit-explorer/tests/unit/codes/test_logicals.py | Adds GF(2) invariant-based tests for the logical operator construction. |
| deltakit-explorer/tests/unit/circuits/test_tableau_functions.py | Updates how the installed stim package version is detected (but currently uses the wrong distribution name). |
| deltakit-explorer/pyproject.toml | Removes ldpc from the explorer package dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from deltakit_explorer.qpu._native_gate_set import NativeGateSet | ||
|
|
||
| CURRENT_STIM_VERSION = Version(version("stim")) | ||
| CURRENT_STIM_VERSION = Version(version("deltakit_stim")) |
There was a problem hiding this comment.
@rolandriver I can action this, though I think the machinery under the hood normalizes hence passing local examples. However I'm not sure what acceptable resolutions are for flaky linkcheck and the pip audit.
….metadata.version call.
…removed as part of this feature.
rolandriver
left a comment
rolandriver
left a comment
There was a problem hiding this comment.
Thank you @jsimbadev for you contribution this is really appreciated. Comments from my side mainly for the sake of understanding.
|
|
||
| CURRENT_STIM_VERSION = Version(version("stim")) | ||
| CURRENT_STIM_VERSION = Version(version("deltakit_stim")) | ||
| STIM_VERSION_V1_13_0 = Version("1.13.0") |
There was a problem hiding this comment.
Thank you for this. There might be some remaining bits and pieces about stim that slipped through my attention. It feels like there are some here. It might be beyond the scope of this PR but if you could flag them in an issue, that'd be great for me to fix them up later.
There was a problem hiding this comment.
Could you please raise an issue pointing at this?
| Returns: | ||
| The pivot row indices of ``check``. | ||
| """ | ||
| # Pivot rows of ``check`` are the pivot columns of ``check.T``. |
There was a problem hiding this comment.
| # Pivot rows of ``check`` are the pivot columns of ``check.T``. | |
| # Pivot rows of ``check`` are the non-zero pivot columns of ``check.T``. |
| The pivot row indices of ``check``. | ||
| """ | ||
| # Pivot rows of ``check`` are the pivot columns of ``check.T``. | ||
| rr = check.T.row_reduce() |
There was a problem hiding this comment.
Maybe more meaningful names than rr or nz.
There was a problem hiding this comment.
You're right! Went for row_reduced and non_zero_row. Coming in next commit
| """ | ||
|
|
||
| def compute_lz( | ||
| def compute_lz_galois( |
There was a problem hiding this comment.
Not sure this function name change is meaningful. I'd rather add this in a docstring that is missing.
There was a problem hiding this comment.
Yup you're right! Adding doc string also.
| # operators_res == operators_ref | ||
|
|
||
|
|
||
| def _binary_matrix(rows: list[list[int]], n_cols: int) -> np.ndarray: |
There was a problem hiding this comment.
I would use NDArray from np.typing to make it more idiomatic to the code base.
| def _assert_logical_basis( | ||
| logicals: np.ndarray, kernel_checks: np.ndarray, image_checks: np.ndarray | ||
| ) -> None: | ||
| assert logicals.ndim == 2 |
There was a problem hiding this comment.
Maybe being more explicit about these asserts.
There was a problem hiding this comment.
You're right. I'll name these variables so asserts read more naturally.
| # CSS logicals should extend the image space into a basis for the kernel. | ||
| assert generated_space == null_space | ||
|
|
||
| expected_num_logicals = _dimension(null_space) - _dimension(image_space) |
There was a problem hiding this comment.
Is this checking the rank-nullity theorem?
There was a problem hiding this comment.
Not exactly...
Note - I renamed image_space to quotient_space, since that seemed closer to its role here.
In this test, null_space is all operators that satisfy one set of checks, and quotient_space is the row space generated by the checks from the other side, i.e. the directions we treat as redundant.
compute_lz is intended to return independent rows that satisfy the first checks, but are not already generated by the second set, so the expected number of rows is dim(null_space) - dim(quotient_space)
| @pytest.mark.parametrize( | ||
| ("hx", "hz"), | ||
| [ | ||
| (_binary_matrix([], 3), _binary_matrix([], 3)), |
There was a problem hiding this comment.
Not too sure if it makes sense to pass an empty list and a ncols to create a binary matrix.
There was a problem hiding this comment.
yup, I've addressed that by splitting those two behaviours. The function was doing too much.
|
@rolandriver Tried to action your comments. Ready for review again |
|
Thank you @jsimbadev. I will review anytime soon. In the meantime, the fix for the failing static checks is to constrain the range of these dependencies: "tomlkit<0.16,>=0.13",
"python-semantic-release >=10.6.1, <11",For checking the links, this is a timeout issue so rerunning it until it works (or until we find a better fix really). |
rolandriver
left a comment
rolandriver
left a comment
There was a problem hiding this comment.
Thank you for this @jsimbadev. I still see np.array in type annotations and return types that need to be replaced by npt.NDArray. Otherwise looking fine.
| ) | ||
|
|
||
|
|
||
| def pivot_rows(check: galois.FieldArray) -> NDArray[np.int_]: |
There was a problem hiding this comment.
| def pivot_rows(check: galois.FieldArray) -> NDArray[np.int_]: | |
| def pivot_rows(parity_check_matrix: galois.FieldArray) -> NDArray[np.int_]: |
There was a problem hiding this comment.
Done. Coming with next commit )
|
|
||
| CURRENT_STIM_VERSION = Version(version("stim")) | ||
| CURRENT_STIM_VERSION = Version(version("deltakit_stim")) | ||
| STIM_VERSION_V1_13_0 = Version("1.13.0") |
There was a problem hiding this comment.
Could you please raise an issue pointing at this?
| logicals: np.ndarray, | ||
| null_space_defining_checks: np.ndarray, | ||
| quotient_space_defining_checks: np.ndarray, | ||
| ) -> None: |
|
raised issue #313 I can move the patch as a fix against that issue. And I can make the suggested pyproject.toml suggestions there against that bug then hopefully get this GF2 PR in cleanly after a rebase.. thoughts @rolandriver |
AndrewPattersonRL
left a comment
AndrewPattersonRL
left a comment
There was a problem hiding this comment.
I've had a look at the implementation - which looks good to me! Thanks for your work on this @jsimbadev
|
Actually, on the tests - there are a small number of examples. I see that this is because the function that re-builds logicals without using the tested code is quite expensive. Could I suggest hard-coding a few examples? You can source some codes from |
|
@jsimbadev Could you please rebase, address the comment about testing by @AndrewPattersonRL and resolve any discussions? Thank you so much 🙏 |
|
Thanks for looking over the code everyone. @AndrewPattersonRL following your suggestion to use actual codes led to find a bug in my implementation. It seems that not just any logical basis will do, after reading the through some of the BBCode logic, and noticed a constraint on structure of the logical basis
@rolandriver @nelimee Would appreciate any advice ) Note: For reproducing, just try to construct a BBCode object from the feature branch. |
|
Thanks, I'm having a look at this now. To your first point, I see that the matrices returned by both are different, I'll dig a bit more to see where the convention is enforced. To your second point, this is a discussion for @rolandriver, @nelimee. When we migrated from the internal |
|
@AndrewPattersonRL Thanks for looking into this, apologies to put an extra thing on your plate. With regards to the testing... In the past where I've had to deal with large or expensive constructions, the approaches often considered were the following
or have a test that calls a lazy BBCode._calculate_logical_operators and checks the above.
@rolandriver @nelimee Look forward to your thoughts Note: However The above dont address the correctness issue. |
|
I've written the following script to basically systematically isolate the point of divergence. I generate kernels via This suggests that the kernel space basis convention difference between the two libraries GF2 and ldpc (mod2) is worth looking into. @AndrewPattersonRL Let me know if you've found anything. I'll continue as I'm going The following can be ran on my feature branch with a
from __future__ import annotations
from dataclasses import dataclass
from functools import reduce
import galois
import numpy as np
import numpy.typing as npt
from ldpc import mod2
def pivot_rows(parity_check_matrix: galois.FieldArray) -> npt.NDArray[np.int_]:
"""Return pivot rows using the feature-branch GF(2) pivot logic."""
row_reduced = parity_check_matrix.T.row_reduce()
pivots: list[int] = []
for row in row_reduced:
non_zero_row = np.flatnonzero(row != 0)
if non_zero_row.size:
pivots.append(int(non_zero_row[0]))
return np.asarray(pivots, dtype=np.int_)
@dataclass(frozen=True)
class BBFixture:
"""Parameters defining one bivariate bicycle ``hx``/``hz`` fixture."""
name: str
param_l: int
param_m: int
m_a_powers: tuple[int, int, int]
m_b_powers: tuple[int, int, int]
@property
def n(self) -> int:
return 2 * self.param_l * self.param_m
FIXTURES = (
BBFixture(
name="small-n30-k8",
param_l=3,
param_m=5,
m_a_powers=(1, 1, 4),
m_b_powers=(0, 1, 2),
),
BBFixture(
name="paper-72-12-6",
param_l=6,
param_m=6,
m_a_powers=(3, 1, 2),
m_b_powers=(3, 1, 2),
),
BBFixture(
name="paper-90-8-10",
param_l=15,
param_m=3,
m_a_powers=(9, 1, 2),
m_b_powers=(0, 2, 7),
),
)
def bb_hx_hz(
fixture: BBFixture,
) -> tuple[npt.NDArray[np.uint8], npt.NDArray[np.uint8]]:
"""Construct ``Hx = [A B]`` and ``Hz = [B.T A.T]`` for a BB fixture."""
shift_l = np.roll(np.eye(fixture.param_l, dtype=np.int_), shift=1, axis=1)
shift_m = np.roll(np.eye(fixture.param_m, dtype=np.int_), shift=1, axis=1)
identity_l = np.eye(fixture.param_l, dtype=np.int_)
identity_m = np.eye(fixture.param_m, dtype=np.int_)
x = np.kron(shift_l, identity_m)
y = np.kron(identity_l, shift_m)
a = (
reduce(
np.add,
(
np.linalg.matrix_power(x, fixture.m_a_powers[0]),
np.linalg.matrix_power(y, fixture.m_a_powers[1]),
np.linalg.matrix_power(y, fixture.m_a_powers[2]),
),
)
% 2
)
b = (
reduce(
np.add,
(
np.linalg.matrix_power(y, fixture.m_b_powers[0]),
np.linalg.matrix_power(x, fixture.m_b_powers[1]),
np.linalg.matrix_power(x, fixture.m_b_powers[2]),
),
)
% 2
)
return (
np.hstack((a, b)).astype(np.uint8),
np.hstack((b.T, a.T)).astype(np.uint8),
)
def full_mod2_path(
_hx: npt.NDArray[np.uint8],
_hz: npt.NDArray[np.uint8],
) -> dict[str, npt.NDArray[np.integer] | int]:
"""Run the original ``ldpc.mod2`` ``compute_lz(_hx, _hz)`` steps."""
kernel = mod2.nullspace(_hx).todense()
log_stack = np.vstack((_hz, kernel))
rank = mod2.rank(_hz)
all_pivots = mod2.pivot_rows(log_stack)
pivots = all_pivots[rank:]
return {
"kernel": kernel,
"log_stack": log_stack,
"rank": rank,
"all_pivots": all_pivots,
"pivots": pivots,
"logicals": log_stack[pivots],
}
def feature_path_with_mod2_kernel(
_hz: npt.NDArray[np.uint8],
kernel_hx: npt.NDArray[np.uint8],
) -> dict[str, npt.NDArray[np.integer] | int]:
"""Run the feature-branch rank/pivot path using a supplied ``ker(_hx)``."""
_hz_gf = galois.GF2(_hz)
kernel_hx_gf = galois.GF2(np.asarray(kernel_hx, dtype=np.int_))
log_stack = np.vstack((_hz_gf, kernel_hx_gf))
rank = int(np.linalg.matrix_rank(_hz_gf))
all_pivots = pivot_rows(log_stack)
pivots = all_pivots[rank:]
return {
"log_stack": log_stack,
"rank": rank,
"all_pivots": all_pivots,
"pivots": pivots,
"logicals": log_stack[pivots],
}
def right_zero_count(matrix: npt.NDArray[np.uint8]) -> int:
"""Return how many rows have zero support in the right block."""
half = matrix.shape[1] // 2
return int(np.sum(np.all(matrix[:, half:] == 0, axis=1)))
def compare_arrays(
old_path: dict[str, npt.NDArray[np.integer] | int],
feature_path: dict[str, npt.NDArray[np.integer] | int],
) -> dict[str, bool]:
"""Compare shared outputs from the two paths."""
return {
key: (
old_path[key] == feature_path[key]
if key == "rank"
else bool(np.array_equal(old_path[key], feature_path[key]))
)
for key in ("log_stack", "rank", "all_pivots", "pivots", "logicals")
}
def report_side(
fixture: BBFixture,
side_name: str,
_hx: npt.NDArray[np.uint8],
_hz: npt.NDArray[np.uint8],
) -> bool:
"""Print the comparison for one logical side."""
old_path = full_mod2_path(_hx, _hz)
feature_path = feature_path_with_mod2_kernel(
_hz=_hz,
kernel_hx=old_path["kernel"],
)
comparisons = compare_arrays(old_path, feature_path)
matched = all(comparisons.values())
print(f" {side_name}:")
print(f" kernel shape: {old_path['kernel'].shape}")
print(f" log_stack shape: {old_path['log_stack'].shape}")
print(f" rank: mod2={old_path['rank']} feature={feature_path['rank']}")
print(f" mod2 pivots: {old_path['pivots'].tolist()}")
print(f" feature pivots:{feature_path['pivots'].tolist()}")
print(f" exact matches: {comparisons}")
print(
" right-zero logical rows: "
f"{right_zero_count(old_path['logicals'])} / "
f"{old_path['logicals'].shape[0]}"
)
if not matched:
print(f" FAILED: {fixture.name} {side_name} did not match exactly.")
return matched
def report_fixture(fixture: BBFixture) -> bool:
"""Print the comparison for one BB fixture."""
hx, hz = bb_hx_hz(fixture)
rank_hx = int(mod2.rank(hx))
rank_hz = int(mod2.rank(hz))
logical_dimension = fixture.n - rank_hx - rank_hz
print(f"\n{fixture.name}")
print(
" parameters: "
f"l={fixture.param_l}, m={fixture.param_m}, "
f"A={list(fixture.m_a_powers)}, B={list(fixture.m_b_powers)}"
)
print(f" shapes: hx={hx.shape}, hz={hz.shape}")
print(f" ranks: rank(hx)={rank_hx}, rank(hz)={rank_hz}, k={logical_dimension}")
print(f" checks commute: {bool(np.all((hx @ hz.T) % 2 == 0))}")
# Match css_code_compute_logicals: lx = compute_lz(hz, hx), lz = compute_lz(hx, hz).
lx_matched = report_side(fixture, "lx = compute_lz(hz, hx)", _hx=hz, _hz=hx)
lz_matched = report_side(fixture, "lz = compute_lz(hx, hz)", _hx=hx, _hz=hz)
return lx_matched and lz_matched
def main() -> None:
"""Run the diagnostic comparison for all fixtures."""
print(f"pivot_rows source: {PIVOT_ROWS_SOURCE}")
all_matched = True
for fixture in FIXTURES:
all_matched = report_fixture(fixture) and all_matched
print("\nSummary")
print(f" all fixtures matched exactly: {all_matched}")
if not all_matched:
raise SystemExit(1)
if __name__ == "__main__":
main() |
🔗 Closed Issues
Link any issues closed or comments addressed by this PR.
"Closes #X" will close issue X automatically when the PR is merged.
"Closes #264 "
📝 Description
This PR replaces the ldpc.mod2 usage in CSS logical-operator construction with galois GF(2) linear algebra in deltakit_explorer.codes._logicals.
Specifically, it replaces the previous ldpc-based:
with a galois-based implementation over GF(2).
The new pivot-row helper was validated against ldpc.mod2.pivot_rows() on small dense binary matrices, and the logical-construction tests were expanded to check GF(2) correctness invariants rather than a specific basis ordering.
The added tests verify that the returned logical operators:
🚦 Status
Ready for review.
One follow-up question is whether the input contract to this path should be tightened further. The current implementation assumes binary parity-check matrices; if non-binary floating inputs were ever passed here, coercion to integer arrays before galois.GF2(...) could silently normalize invalid inputs instead of rejecting them.
🛠️ Future Work
Potential follow-ups:
➕️ Additional Information
Anything that doesn't fit into a category above.
🧾 Release Note
If this PR will be squash-merged, draft the final one-line commit message here.
Otherwise, describe your rebase strategy or how you plan to maintain clean commits.
replace ldpc GF(2) logical-operator linear algebra with galois