Give repr to contrib.routing.SwapNetwork, add error message test for unmapped qubits #2251
Conversation
# Conflicts: # cirq/contrib/acquaintance/permutation.py # cirq/contrib/routing/swap_network_test.py
| b: b | ||
| }) | ||
| with pytest.raises(ValueError, match='acts on unmapped qubit'): | ||
| _ = list(swap_network.get_logical_operations()) |
There was a problem hiding this comment.
Is the mock assignment for pylint?
There was a problem hiding this comment.
Yes, sometimes pylint complains that the result of an expression is unused. So I got into the habit of assigning to _ as a way of saying "I know I'm throwing this away".
There was a problem hiding this comment.
Maybe
for op in swap_network.get_logical_operations():
pass # coverage: ignore
It's fine as-is if you have a preference.
| if (isinstance(op, ops.GateOperation) and | ||
| isinstance(op.gate, PermutationGate)): | ||
| op.gate.update_mapping(mapping, op.qubits) | ||
| # Ignoring type warning about passing an abstract type into Type[...]. |
There was a problem hiding this comment.
Maybe link to the issue here?
| isinstance(op.gate, PermutationGate)): | ||
| op.gate.update_mapping(mapping, op.qubits) | ||
| # Ignoring type warning about passing an abstract type into Type[...]. | ||
| gate = ops.op_gate_of_type(op, PermutationGate) # type: ignore |
There was a problem hiding this comment.
Is this just a style preference?
There was a problem hiding this comment.
I can revert it if you prefer. It's just much shorter, and it's why we defined these methods.
There was a problem hiding this comment.
I don't have a preference. IIRC, this is what I originally wrote, and changed it to avoid the type issues.
| gate = ops.op_gate_of_type(op, PermutationGate) # type: ignore | ||
| if gate is not None: | ||
| # Ignoring type warning about op.qubits not being a tuple. | ||
| gate.update_mapping(mapping, op.qubits) # type: ignore |
There was a problem hiding this comment.
Why does mypy think op.qubits has type Union[Tuple[Qid, ...], FrozenSet[Qid]]? Shouldn't it be a tuple?
There was a problem hiding this comment.
I honestly don't know. My guess is that somewhere in the code base a qubits method returns a FrozenSet[Qid] and mypy is inferring the return type from that, but I wasn't able to find where.
There was a problem hiding this comment.
It seems like a silly thing to spend too much time on, but this seems like a case where mypy might actually be catching an bug, so maybe we shouldn't override it so quickly.
|
I don't want to hold this up on the mypy thing; if you think it's not worth the trouble, that's fine. LGTM after adding the link to the issue. |
|
@Strilanc ping |
Strilanc
added
the
automerge
|
Automerge cancelled: No approved review. |
CirqBot
removed
the
automerge
|
I thought it was good modulo getting the tests passed. |
|
@Strilanc - can you have a look at this why it's failing and merge it? |
|
Closed as obsolete |
No description provided.