Add a phased classical action for SelectedMajoranaFermion

[maxglick]

Add a phased classical action for SelectedMajoranaFermion.
The classical action only exists for some choices of target_gate, and we assume specifically target_gate=cirq.X (and also that there is only 1 control register and 1 selection register).

Fixes #1699

[mpharrigan]

is this restriction necessary?

[maxglick]

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?

[mpharrigan]

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)

[maxglick]

I have now added Z.

[mpharrigan]

can you add a comment describing how this logic works

[maxglick]

Done.

[mpharrigan]

Can you be even more explicit with how the bit twiddling operations correspond to the promised action of the subroutine

"flip the selection-th bit in target. the selection-th bit is addressed with the unsigned integer 2^(N - i) in our big endian convention"

or something like that

[mpharrigan]

/gemini review

[gemini-code-assist]

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.

[gemini-code-assist]

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.

Suggested change

	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

[gemini-code-assist]

If selection exceeds max_selection, max_selection - selection becomes negative, causing 2 ** (max_selection - selection) to return a float. Bitwise XOR (^) with a float will then raise a TypeError. We should ensure selection <= max_selection before applying the bit flip. If the selection is out of range, the gate should act as identity.

Suggested change

	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
	max_selection = self.selection_registers[0].dtype.iteration_length_or_zero() - 1
	if control and self.target_gate == cirq.X and selection <= max_selection:
	target = (2 ** (max_selection - selection)) ^ target

[gemini-code-assist]

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.

Suggested change

	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

[gemini-code-assist]

If selection exceeds max_selection, the bit shift count max_selection - selection can become negative, leading to a ValueError. Additionally, if selection == max_selection + 1 and target_gate == cirq.X, the shift count is 0, which would incorrectly count all bits in target. The phase should only be calculated when selection <= max_selection.

Suggested change

	if control:
	max_selection = self.selection_registers[0].dtype.iteration_length_or_zero() - 1
	if self.target_gate == cirq.X:
	num_phases = (target >> (max_selection - selection + 1)).bit_count()
	else:
	num_phases = (target >> (max_selection - selection)).bit_count()
	return 1 if (num_phases % 2) == 0 else -1
	max_selection = self.selection_registers[0].dtype.iteration_length_or_zero() - 1
	if control and selection <= max_selection:
	if self.target_gate == cirq.X:
	num_phases = (target >> (max_selection - selection + 1)).bit_count()
	else:
	num_phases = (target >> (max_selection - selection)).bit_count()
	return 1 if (num_phases % 2) == 0 else -1
	return 1

[mpharrigan]

explain how this works

[mpharrigan]

explain how this works and why it's different from the cirq.X case. Maybe be more defensive and put this behind an elif in case we add more in the future e.g. cirq.Y

[mpharrigan]

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