Introduce XZFaultList for tracking X and Z faults simultaneously - #783
Introduce XZFaultList for tracking X and Z faults simultaneously#783sunjerry019 wants to merge 23 commits into
Conversation
Assisted by (Auto) via GitHub Copilot
…but should be before it
|
@coderabbitai full review |
✅ Action performedFull review finished. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds public ChangesXZ Fault List
Estimated code review effort: 4 (Complex) | ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/mqt/qecc/circuit_synthesis/faults.py`:
- Around line 562-571: Update XZFaultList.add_faults in
src/mqt/qecc/circuit_synthesis/faults.py at lines 562-571 to validate both
inputs are 2D before accessing shape[1], and raise ValueError for 1D inputs;
then validate equal row counts before either np.vstack call and raise ValueError
when they differ. Add tests in tests/circuit_synthesis/test_faults.py at lines
713-752 covering mismatched row counts and 1D input, each asserting ValueError.
- Around line 771-789: Rename ensure_apply_valid_input to
_ensure_apply_valid_input and update every apply_* caller to use the private
helper name. Remove the boolean return contract and the final True return,
allowing the method to return None after successful validation while preserving
its existing ValueError checks.
- Around line 566-568: Update add_faults to validate that both x_faults and
z_faults are at least two-dimensional before accessing shape[1], raising the
documented ValueError for invalid dimensions; retain the existing num_qubits
column-count validation for valid-dimensional inputs.
- Around line 819-832: Update the fault-reduction method around ret, generators,
and coset_leader to validate and normalize both generator arrays before
modifying any faults, preserving atomic behavior for inplace=True. Pair
generators explicitly with the corresponding "X" and "Z" fault keys rather than
relying on zip iteration order, then perform reductions using those key
mappings. Add the runtime warning documented for
PureFaultSet.faults_to_coset_leaders to indicate that coset_leader is invoked
once per fault row.
In `@tests/circuit_synthesis/test_faults.py`:
- Around line 713-752: Update XZFaultList.add_faults validation to require X and
Z arrays have matching row counts, raising the documented ValueError before
appending data. Also validate that supplied arrays are two-dimensional so 1D
inputs raise ValueError rather than IndexError. Add tests alongside
test_add_faults_rejects_wrong_column_count covering mismatched rows and 1D
arrays.
- Around line 1038-1043: Update the assertions for the non-matching faults in
the relevant tests to compare the reduced fault rows against their expected
coset leaders, specifically asserting the deterministic zero vector for the
described X fault and checking index 2 in the second test. Replace the
non-informative row-count-only assertions while preserving the existing zero
checks for matching faults.
- Around line 904-951: Extend test_apply_ccz_unit_tests with at least one case
using a nonzero input Z on a control, and update expected Z/output assertions to
verify XOR cancellation behavior. Extend test_apply_ccx_unit_tests to exercise
inplace=True, asserting the returned object is the original faults instance and
that its X and Z data are mutated to the expected values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 714d31b5-134e-4e19-af37-4cee20bba0c8
📒 Files selected for processing (2)
src/mqt/qecc/circuit_synthesis/faults.pytests/circuit_synthesis/test_faults.py
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Co-authored by GPT-5.4 mini via Github Copilot
…hem explicitly." Co-authored by GPT5.4-mini via Github Copilot
|
@lsschmid Could you look over this, and if it's okay, merge it? Thank you! |
|
I'll try to find the time the next days 👍 |
lsschmid
left a comment
lsschmid
left a comment
There was a problem hiding this comment.
A few small things, mostly about staying consistent with PureFaultSet. Still thinking about the CCZ propagation model, will come back to that separately.
|
|
||
| assert len(generators) == 2, "Generators should be a tuple of x_generators and z_generators" | ||
|
|
||
| logger.warning("This calls coset_leader once per fault row, so it might take a while.") |
There was a problem hiding this comment.
This fires on every call. The docstring already has a Warning section for it, so I'd either drop it or downgrade to logger.debug.
|
|
||
| return ret | ||
|
|
||
| def __repr__(self) -> str: |
There was a problem hiding this comment.
object.__repr__ puts the memory address in here, so the output isn't stable across runs. PureFaultSet.__repr__ just above returns a clean string, can we match that?
| if inplace: | ||
| # Apply CNOT directly to self.faults | ||
| x_faults, z_faults = self.faults["X"], self.faults["Z"] | ||
| ret = self | ||
| else: | ||
| # Create a new XZFaultList with copied faults | ||
| new_list = XZFaultList(self.num_qubits) | ||
| new_list.faults["X"] = np.copy(self.faults["X"]) | ||
| new_list.faults["Z"] = np.copy(self.faults["Z"]) | ||
|
|
||
| x_faults, z_faults = new_list.faults["X"], new_list.faults["Z"] | ||
| ret = new_list |
There was a problem hiding this comment.
This copy-or-self block is repeated in apply_hadamard, apply_reset and apply_ccz. reduce_to_coset_leaders already does it as ret = self if inplace else self.copy(), would be nice to use that everywhere.
| assert fault_set != PureFaultSet.from_fault_array(faults), "Faults were not permuted correctly in place" | ||
|
|
||
|
|
||
| """XZFaultList Tests""" |
There was a problem hiding this comment.
This is a no-op expression, not a docstring. Make it a # comment.
| self.faults = { | ||
| "X": np.zeros((0, num_qubits), dtype=np.int8), | ||
| "Z": np.zeros((0, num_qubits), dtype=np.int8), | ||
| } |
There was a problem hiding this comment.
The "X"/"Z" keys are typed as a plain dict[str, ...], so a typo like faults["Y"] only blows up at runtime. Two attributes (x_faults/z_faults, like the naming in state_prep.py) or a TypedDict would be safer. The tests also reach into faults.faults["X"] everywhere, so an accessor might be worth it.
| new_list.faults["Z"] = np.copy(self.faults["Z"]) | ||
| return new_list | ||
|
|
||
| def __iter__(self) -> Iterator[tuple[npt.NDArray[np.int8], npt.NDArray[np.int8]]]: |
There was a problem hiding this comment.
There's __iter__ but no __len__. Also no __eq__, to_array or from_fault_array like PureFaultSet has. __eq__ especially will be needed as soon as something actually uses this class.
| for i in range(len(self.faults["X"])): | ||
| yield (self.faults["X"][i], self.faults["Z"][i]) | ||
|
|
||
| def apply_cnot(self, control: int, target: int, inplace: bool = True) -> XZFaultList: |
There was a problem hiding this comment.
inplace defaults to True here, but PureFaultSet.combine defaults to False. Can we settle on one default for the module?
| ValueError: If fault arrays don't have the correct length. | ||
| ValueError: If both faults are None | ||
| """ | ||
| assert len(faults) == 2, "Faults should be a tuple of x_fault and z_fault" |
There was a problem hiding this comment.
The type hint already fixes this to a 2-tuple, and asserts get stripped under -O. Same for the one in reduce_to_coset_leaders. Drop them, or raise a ValueError if you want the check to be real?
|
Hey! In general this looks good to me, thanks a lot for putting it together. One thing: I don't know exactly what you need this for, but for the CCZ model some things are unclear to me.
Second, the if that is clarified I guess we can merge |
2 participants
Description
Expands on the idea of$X$ - and $Z$ -Faults on which gates such as CNOTs, H, Reset may be applied.
PureFaultSetby having a list (i.e. stable order) of coupledThis PR is a fresh copy of the draft PR #712 without the edits to
PureFaultSet, so that it can already be merged into main. Commits were picked from the branch XZFaultList.In this case #690 need not be merged.
Checklist
I have updated the documentation to reflect these changes.If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.