Add function to apply CNOTs to a PureFaultSet#690
Open
sunjerry019 wants to merge 26 commits into
Open
Conversation
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Co-authored-by: Copilot <copilot@github.com>
…ultSet from a fault_array
…eFaultSet_Extension
9 tasks
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
📝 WalkthroughSummary by CodeRabbitRelease Notes
Walkthrough
ChangesFault kind tracking and CNOT transformation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/mqt/qecc/circuit_synthesis/faults.py (4)
270-278:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
copydoes not propagatekind.The copy always has
kind="X"regardless of the original's kind, which will cause incorrect behavior if the copy is later used withapply_cnot.🐛 Proposed fix
def copy(self) -> PureFaultSet: """Create a copy of the PureFaultSet. Returns: A new PureFaultSet object with the same faults and number of qubits. """ - new_set = PureFaultSet(self.num_qubits) + new_set = PureFaultSet(self.num_qubits, kind=self.kind) new_set.faults = np.copy(self.faults) return new_set🤖 Prompt for 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. In `@src/mqt/qecc/circuit_synthesis/faults.py` around lines 270 - 278, The PureFaultSet.copy method currently constructs a new_set with PureFaultSet(self.num_qubits) which defaults kind to "X", so the original object's kind is lost; update copy to propagate the kind and any other non-default attributes by assigning new_set.kind = self.kind (and copy any additional metadata if present) before returning, and ensure faults are still copied via np.copy(self.faults) as already done.
366-370:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
filter_faultsdoes not propagatekindwheninplace=False.The returned
PureFaultSetwill havekind="X"regardless of the original's kind.🐛 Proposed fix
if inplace: self.faults = filtered return self - return PureFaultSet.from_fault_array(filtered) + return PureFaultSet.from_fault_array(filtered, kind=self.kind)🤖 Prompt for 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. In `@src/mqt/qecc/circuit_synthesis/faults.py` around lines 366 - 370, The filter_faults branch that returns a new PureFaultSet doesn't preserve the original set's kind (so it always ends up as "X"); update the return path in filter_faults to propagate self.kind to the new object—either by calling PureFaultSet.from_fault_array(filtered, kind=self.kind) if that factory accepts a kind parameter or by creating the PureFaultSet and explicitly setting its .kind = self.kind before returning; ensure the inplace branch still behaves unchanged.
88-91:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
combinedoes not propagatekindand ignores kind mismatch.When
inplace=False, the returnedPureFaultSetalways haskind="X"(the default) regardless of the original fault sets' kinds. Additionally, combining fault sets of different kinds (e.g., X and Z) should likely raise an error or at least propagate a consistent kind.🐛 Proposed fix to propagate kind and validate consistency
def combine(self, other: PureFaultSet, inplace: bool = False) -> PureFaultSet: ... if self.num_qubits != other.num_qubits: msg = "Fault sets must have the same number of qubits to combine." raise ValueError(msg) + if self.kind != other.kind: + msg = "Fault sets must have the same kind to combine." + raise ValueError(msg) combined_faults = np.vstack([self.faults, other.faults]) if inplace: self.faults = combined_faults return self - return PureFaultSet.from_fault_array(combined_faults) + return PureFaultSet.from_fault_array(combined_faults, kind=self.kind)🤖 Prompt for 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. In `@src/mqt/qecc/circuit_synthesis/faults.py` around lines 88 - 91, The combine method currently returns a new PureFaultSet with default kind "X" when inplace=False and does not validate mixed kinds; update the combine implementation (the combine function that builds combined_faults and calls PureFaultSet.from_fault_array) to (1) determine the resultant kind by inspecting self.kind and other.kind, (2) if kinds differ raise a ValueError (or explicit exception) indicating a kind mismatch, and (3) when constructing the returned PureFaultSet, pass or set the resolved kind so the new PureFaultSet preserves the correct kind instead of always defaulting to "X"; if inplace=True, ensure you also validate kinds before assigning self.faults and preserve self.kind.
387-391:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
permute_qubitsdoes not propagatekindwheninplace=False.The returned
PureFaultSetwill havekind="X"regardless of the original's kind.🐛 Proposed fix
if inplace: self.faults = permuted_faults return self - return PureFaultSet.from_fault_array(permuted_faults) + return PureFaultSet.from_fault_array(permuted_faults, kind=self.kind)🤖 Prompt for 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. In `@src/mqt/qecc/circuit_synthesis/faults.py` around lines 387 - 391, permute_qubits currently returns a new PureFaultSet via PureFaultSet.from_fault_array(permuted_faults) without preserving the source object's kind, so when inplace=False the new set ends up with kind="X"; update permute_qubits to propagate the original kind (e.g. self.kind) to the returned PureFaultSet — either by calling a constructor/factory that accepts a kind argument or by setting returned.kind = self.kind immediately after creation — while keeping the existing inplace behavior intact.
🤖 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 404-409: The current validation in the block that checks control
and target against self.num_qubits allows negative indices due to NumPy negative
indexing; update the check in the method containing control/target (the block
referencing control, target, and self.num_qubits) to also reject negative values
(e.g., ensure control >= 0 and target >= 0) before the existing upper-bound
checks and raise a ValueError with a clear message like "Control and target
indices must be between 0 and {self.num_qubits - 1}." if either is negative or
out of range; keep the existing distinct check that control != target.
In `@tests/circuit_synthesis/test_faults.py`:
- Around line 713-722: Extend the test_apply_cnot_invalid_qubits case to also
assert that negative qubit indices raise the same ValueError: call
fault_set.apply_cnot(control=-1, target=1) and expect the "Control and target
indices must be between 0 and 2." message; update the test function
(test_apply_cnot_invalid_qubits) so it includes this additional with
pytest.raises check against the existing PureFaultSet.apply_cnot usage.
---
Outside diff comments:
In `@src/mqt/qecc/circuit_synthesis/faults.py`:
- Around line 270-278: The PureFaultSet.copy method currently constructs a
new_set with PureFaultSet(self.num_qubits) which defaults kind to "X", so the
original object's kind is lost; update copy to propagate the kind and any other
non-default attributes by assigning new_set.kind = self.kind (and copy any
additional metadata if present) before returning, and ensure faults are still
copied via np.copy(self.faults) as already done.
- Around line 366-370: The filter_faults branch that returns a new PureFaultSet
doesn't preserve the original set's kind (so it always ends up as "X"); update
the return path in filter_faults to propagate self.kind to the new object—either
by calling PureFaultSet.from_fault_array(filtered, kind=self.kind) if that
factory accepts a kind parameter or by creating the PureFaultSet and explicitly
setting its .kind = self.kind before returning; ensure the inplace branch still
behaves unchanged.
- Around line 88-91: The combine method currently returns a new PureFaultSet
with default kind "X" when inplace=False and does not validate mixed kinds;
update the combine implementation (the combine function that builds
combined_faults and calls PureFaultSet.from_fault_array) to (1) determine the
resultant kind by inspecting self.kind and other.kind, (2) if kinds differ raise
a ValueError (or explicit exception) indicating a kind mismatch, and (3) when
constructing the returned PureFaultSet, pass or set the resolved kind so the new
PureFaultSet preserves the correct kind instead of always defaulting to "X"; if
inplace=True, ensure you also validate kinds before assigning self.faults and
preserve self.kind.
- Around line 387-391: permute_qubits currently returns a new PureFaultSet via
PureFaultSet.from_fault_array(permuted_faults) without preserving the source
object's kind, so when inplace=False the new set ends up with kind="X"; update
permute_qubits to propagate the original kind (e.g. self.kind) to the returned
PureFaultSet — either by calling a constructor/factory that accepts a kind
argument or by setting returned.kind = self.kind immediately after creation —
while keeping the existing inplace behavior intact.
🪄 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
Run ID: 20c121e2-eb38-4d11-8a50-b4c52a9a9776
📒 Files selected for processing (2)
src/mqt/qecc/circuit_synthesis/faults.pytests/circuit_synthesis/test_faults.py
… __eq__ also checks kind
…ion' into PureFaultSet_Extension
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
"Pushing" fault sets through a CNOT circuit is a commonly-used operation, implementing this function to a PureFaultSet seems like a good quality-of-life improvement. A PureFaultSet now also remembers what kind of error set it represents.
Assisted-by: (Auto) via GitHub Copilot through inline suggestions and reviewed by me.
Checklist
I have added migration instructions to the upgrade guide (if needed).not relevantIf PR contains AI-assisted content:
Coauthored-by: [Model Name] via [Tool Name]footer.