Conversation
… selection and/or control registers.
![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 updates the SelectedMajoranaFermion class to support multiple control and selection registers in both on_classical_vals and basis_state_phase, along with adding a test case for multiple selection registers. The review feedback correctly identifies that checking for active controls using simple equality against zero can raise a ValueError for multi-dimensional arrays and fails to handle control registers with a bitsize greater than one. The reviewer provides robust code suggestions to resolve these issues using NumPy array checks.
| for control_register in self.control_registers: | ||
| if vals[control_register.name] == 0: | ||
| return vals |
There was a problem hiding this comment.
Checking vals[control_register.name] == 0 will raise a ValueError: The truth value of an array with more than one element is ambiguous if the control register has a non-empty shape (since vals[control_register.name] will be a numpy array). Additionally, if a control register has bitsize > 1, the gate is only active when all control qubits are 1 (i.e., the value is 2**bitsize - 1). We should check if any of the control qubits are inactive by comparing the array/scalar against 2**bitsize - 1 using np.any.
| for control_register in self.control_registers: | |
| if vals[control_register.name] == 0: | |
| return vals | |
| for control_register in self.control_registers: | |
| if np.any(np.asarray(vals[control_register.name]) != (2 ** control_register.bitsize - 1)): | |
| return vals |
There was a problem hiding this comment.
you can test this by constructing a SelectedMajorannaFermion bloq with a shaped control register
| for control_register in self.control_registers: | ||
| if vals[control_register.name] == 0: | ||
| return 1 |
There was a problem hiding this comment.
Checking vals[control_register.name] == 0 will raise a ValueError: The truth value of an array with more than one element is ambiguous if the control register has a non-empty shape (since vals[control_register.name] will be a numpy array). Additionally, if a control register has bitsize > 1, the gate is only active when all control qubits are 1 (i.e., the value is 2**bitsize - 1). We should check if any of the control qubits are inactive by comparing the array/scalar against 2**bitsize - 1 using np.any.
| for control_register in self.control_registers: | |
| if vals[control_register.name] == 0: | |
| return 1 | |
| for control_register in self.control_registers: | |
| if np.any(np.asarray(vals[control_register.name]) != (2 ** control_register.bitsize - 1)): | |
| return 1 |
|
While looking at the AI comments I added a test for multiple controls which is failing with "Tried to free a non-zero register". I am guessing the issue is with the free call at https://github.com/quantumlib/Qualtran/blob/main/qualtran/bloqs/multiplexers/selected_majorana_fermion.py#L111. Any suggestions? |
Update: I am past that error but I am still looking at the AI comments. |
| selection = 0 | ||
| for selection_register in self.selection_registers: | ||
| selection = ( | ||
| selection * (selection_register.dtype.iteration_length_or_zero()) | ||
| + vals[selection_register.name] | ||
| ) |
There was a problem hiding this comment.
what's this doing? is there a more idiomatic way to express it?
There was a problem hiding this comment.
I found np.ravel_multi_index for this from the same file. Thanks for the tip.
| if control and self.target_gate == cirq.X: | ||
| max_selection = self.selection_registers[0].dtype.iteration_length_or_zero() - 1 | ||
| target = (2 ** (max_selection - selection)) ^ target | ||
| if self.target_gate == cirq.X: |
There was a problem hiding this comment.
is the idea that you early-exit for any ctrl!=1? worth adding a line comment describing this logic
There was a problem hiding this comment.
Yes, that's right. I added a comment.
2 participants
Handle multiple selection and/or control registers (while still restricting to target gate X or Z).
Also fix an error in decompose_from_registers where the number of controls was assumed to be 0 or 1.
Fixes #1699 .