Add a phased classical action for SelectedMajoranaFermion#1778
Add a phased classical action for SelectedMajoranaFermion#1778mpharrigan merged 16 commits intoquantumlib:mainfrom
Conversation
Merge with upstream.
| if len(self.control_registers) > 1 or len(self.selection_registers) > 1: | ||
| return NotImplemented |
There was a problem hiding this comment.
is this restriction necessary?
There was a problem hiding this comment.
I'm not sure - it is hard for me to understand what this gate does in the general case. Is my understanding in #1699 (comment) correct?
| if self.target_gate != cirq.X: | ||
| return NotImplemented |
There was a problem hiding this comment.
In combination with basis_state_phase, this should be able to support Z as well. Z imparts a phase (and leaves the computational basis states unaffected)
There was a problem hiding this comment.
I have now added Z.
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements classical simulation support for the SelectedMajoranaFermion bloq by adding on_classical_vals and basis_state_phase methods, along with a new testing utility assert_consistent_phased_classical_action to verify these actions against bloq decompositions. The review feedback identifies several potential runtime issues, including IndexError risks when control or selection registers are empty, and TypeError or ValueError risks during bitwise operations if the selection index exceeds the maximum allowed value. Suggestions were provided to enforce strict register counts and add bounds checking for selection indices.
| if len(self.control_registers) > 1 or len(self.selection_registers) > 1: | ||
| return NotImplemented |
There was a problem hiding this comment.
The current check len(self.control_registers) > 1 does not account for the case where there are zero control registers. If control_regs is empty, accessing self.control_registers[0] on line 146 will raise an IndexError. Given the implementation assumes exactly one control and one selection register, it's safer to check for != 1.
| if len(self.control_registers) > 1 or len(self.selection_registers) > 1: | |
| return NotImplemented | |
| if len(self.control_registers) != 1 or len(self.selection_registers) != 1: | |
| return NotImplemented |
| if len(self.control_registers) > 1 or len(self.selection_registers) > 1: | ||
| return None |
There was a problem hiding this comment.
Similar to on_classical_vals, this check should ensure exactly one control register is present to avoid an IndexError on line 165 when control_regs is empty.
| if len(self.control_registers) > 1 or len(self.selection_registers) > 1: | |
| return None | |
| if len(self.control_registers) != 1 or len(self.selection_registers) != 1: | |
| return None |
mpharrigan
left a comment
mpharrigan
left a comment
There was a problem hiding this comment.
I added some requests for more comments about the bit twiddling. Ideally, someone should be able to read this method and immediately square the circle with the dirac notation definition given in the class docstring.
Re: multiple controls and multiple selection registers: further comments below, but we can do them in a follow up.
Re: multiple controls: they're implicitly "anded together"
Re: multiple selection registers: you can see in the definition of the target register that we're packing an n-dimensional array of bits into the one QAny(prod(*shape)) register. Whereas the current implementation is doing target[i] ^= 1 multiple selection registers would do target[i,j,k] ^= 1
This ties into my review comments where it's not immediately obvious that we're doing target[i]^=1
|
Looks good, Thanks! |
2 participants
Add a phased classical action for SelectedMajoranaFermion.
See #1699 for details.
The classical action only exists for some choices of
target_gate, and we assume specificallytarget_gate=cirq.Xortarget_gate=cirq.Z. We also assume that there is only 1 control register and 1 selection register.