BooleanHamiltonian.with_qubits is broken

[cduck]

Description of the issue
with_qubits is meant to return a new operation with its (ordered) qubits substituted with a new list of qubits. BooleanHamiltonian.with_qubits casts its argument to NamedQubit and otherwise fails (throws AttributeError).

BooleanHamiltonian.with_qubits also assumes the names of the named qubits correspond to the names of the boolean variables. This won't be the case in general, especially if the user is constructing a larger circuit.

How to reproduce the issue

1. with_qubits(*non_named_qubits):

   q0, q1, q2 = cirq.LineQubit.range(3)
   op = cirq.BooleanHamiltonian(
           {'a': q0, 'b': q1, 'c': q2},
           ['a|b|c', 'a^b', 'b^c'],
           0.1,
       )
   op.with_qubits(*cirq.GridQubit.rect(1, 3))  # Throws Attribute Error

2. with_qubits(mismatched_named_qubits):

   q0, q1, q2 = cirq.LineQubit.range(3)
   op = cirq.BooleanHamiltonian(
           {'a': q0, 'b': q1, 'c': q2},
           ['a|b|c', 'a^b', 'b^c'],
           0.1,
       )
   
   # with_qubits doesn't fail but now the variable names in qubit_map don't match
   # the boolean formulas and raise `KeyError` on decompose.
   op2 = op.with_qubits(*cirq.NamedQubit.range(3, prefix='x'))  # Succeeds, but invalid op
   cirq.Circuit(cirq.decompose_once(op2))  # Throws KeyError

Cirq version
0.12.0.dev (after #4309)

[cduck]

Related to this, I would expect this identity to hold for any operation type but BooleanHamiltonian does not:

op.with_qubits(*op.qubits) == op

[balopat]

Thanks for opening this @cduck!
@tonybruguier can you have a look at this please?

[tonybruguier]

Ack

[tonybruguier]

Apologies for the mistake. I sent #4396 as a proposal for a fix.

@cduck I tried to look around what the exact definition should be, but I can of course change my code.

[cduck]

It looks like the documentation could be more descriptive about how with_qubits should work. My understanding is that with_qubits returns a new operations with the exact same effect (i.e. unitary, decomposition, etc.) but on a different list of qubits (only if they have matching dimensions).

I think the fix is much better but it does implicitly rely on key order in dict. I don't know how to avoid this with the current with_qubits API. The only concern I have with the fix is this false equality:

q0, q1 = cirq.LineQubit.range(2)
op1 = cirq.BooleanHamiltonian(
        {'a': q0, 'b': q1},
        ['a'],
        0.1,
    )
op2 = cirq.BooleanHamiltonian(
        {'b': q1, 'a': q0},  # Same mapping but in a different order
        ['a'],
        0.1,
    )

new_qubits = cirq.NamedQubit.range(2, prefix='q')
new_op1 = op1.with_qubits(*new_qubits)  # Map: {'a': cirq.NamedQubit('q0'), 'b': cirq.NamedQubit('q1')}
new_op2 = op2.with_qubits(*new_qubits)  # Map: {'b': cirq.NamedQubit('q0'), 'a': cirq.NamedQubit('q1')}
assert new_op1 != new_op2  # These represent the same Hamiltonian but the qubits are in different orders

# Should these still be treated as equal?
assert op1 == op2

[balopat]

Sorry for pushing - I haven't seen your comment here @cduck - technically the implementation is now correct based on the documentation as the order of qubits is determined by the map insertion order.

However, I see your point regarding insertion order being counterintuitive - but it doesn't just impact with_qubits but also qubits. I would recommend opening a different issue to discuss this - there are multiple avenues:

• maybe we need to pass in an optional qubit_order argument which should be checked against the map values and would control both qubits and with_qubits
• maybe we should force a different qubit ordering - natural ordering of the qubits? They are sortable, right?

[cduck]

Yes and thanks for the fix.

I don't mean to be so pedantic about the API. I don't think the ordering is causing problems right now. If I find it does, I'll open an issue.