[CQT-414] mip mapper maps circuits that do not need to be remapped #659
Conversation
…not need to be remapped
elenbaasc
left a comment
elenbaasc
left a comment
There was a problem hiding this comment.
Thanks for the effort! It seems it was a bit more work that I had anticipated :)
Mostly minor comments. Mainly in regards to the implementation of the map method where you default to using numpy arrays, when simple objects would suffice or are expected in the end anyway, e.g. creating a numpy array and at the end converting it to a list.
| if self.data != other.data: | ||
| return False |
There was a problem hiding this comment.
This is redundant if you follow it with return self.data == other.data
| if self.data != other.data: | |
| return False |
| @@ -1,7 +1,8 @@ | |||
| # OpenQL MIP-Like Mapper | |||
| # OpenQL MIP-Like Mapper. This mapper pass takes inspiration from: | |||
There was a problem hiding this comment.
Is based on?
"... takes inspiration from" is a bit vague.
| self, ir: IR, distance: list[list[int]], num_virtual_qubits: int, num_physical_qubits: int | ||
| ) -> list[list[int]]: | ||
| def _get_reference_counter(self, ir: IR, num_virtual_qubits: int) -> list[list[int]]: | ||
| reference_counter = [[0 for _ in range(num_virtual_qubits)] for _ in range(num_virtual_qubits)] |
There was a problem hiding this comment.
Why does the following replacement not work in this case? (it seems to do the same thing, but when I run the tests they fail..., which is a bit dodgy.)
| reference_counter = [[0 for _ in range(num_virtual_qubits)] for _ in range(num_virtual_qubits)] | |
| reference_counter = [[0] * num_virtual_qubits] * num_virtual_qubits |
| def _get_cost( | ||
| self, ir: IR, distance: list[list[int]], num_virtual_qubits: int, num_physical_qubits: int | ||
| ) -> list[list[int]]: | ||
| def _get_reference_counter(self, ir: IR, num_virtual_qubits: int) -> list[list[int]]: |
There was a problem hiding this comment.
This is a static method:
| def _get_reference_counter(self, ir: IR, num_virtual_qubits: int) -> list[list[int]]: | |
| @staticmethod | |
| def _get_reference_counter(ir: IR, num_virtual_qubits: int) -> list[list[int]]: |
| ) -> list[list[int]]: | ||
| def _get_reference_counter(self, ir: IR, num_virtual_qubits: int) -> list[list[int]]: | ||
| reference_counter = [[0 for _ in range(num_virtual_qubits)] for _ in range(num_virtual_qubits)] | ||
| for statement in getattr(ir, "statements", []): |
There was a problem hiding this comment.
The following should be fine, right?
| for statement in getattr(ir, "statements", []): | |
| for statement in ir.statements: |
| ] | ||
|
|
||
| @staticmethod | ||
| def _get_integrality_and_bounds(num_x_vars: int, num_w_vars: int) -> tuple[np.ndarray, Bounds]: |
There was a problem hiding this comment.
Also here, the integrality is returned as a numpy array of booleans, but where it is used it expects a list of integers. Why not define it as a list of integers from the start?
| reference_counter = self._get_reference_counter(ir, qubit_register_size) | ||
|
|
||
| cost, constraints, integrality, bounds = self._get_linearized_formulation( | ||
| reference_counter, distance, qubit_register_size, num_physical_qubits |
There was a problem hiding this comment.
The qubit_register_size and num_physical_qubits can probably be defined as object attributes, i.e. self.qubit_register_size and self.num_physical_qubits, then they don't need to be passed around.
Same goes for num_virtual_qubits * num_physical_qubits, which is used in various places.
| for i in range(num_virtual_qubits): | ||
| k = int(np.argmax(x_sol[i])) | ||
| mapping.append(k) | ||
|
|
||
| return mapping |
There was a problem hiding this comment.
| for i in range(num_virtual_qubits): | |
| k = int(np.argmax(x_sol[i])) | |
| mapping.append(k) | |
| return mapping | |
| return list(map(lambda x: int(np.argmax(x)), x_sol)) |
| initial_circuit = str(circuit1) | ||
| circuit1.map(mapper=mapper1) | ||
| assert str(circuit1) != initial_circuit | ||
| assert str(circuit1) == initial_circuit |
There was a problem hiding this comment.
I would prefer a different mapping to be the result of a test_map_method. Nevertheless, there should also be a test where the map method is applied, and as expected, the mapping remains the same (just like you did in the test_controlled_gates).
| def test_get_mapping(self, mapper: IdentityMapper, circuit: Circuit) -> None: | ||
| mapping = mapper.map(circuit.ir, circuit.qubit_register_size) | ||
| assert mapping == Mapping([0, 1, 2]) | ||
| assert mapping.items() == Mapping([0, 1, 2]).items() |
There was a problem hiding this comment.
Is this still needed, now that you updated the Mapping.__eq__ method? Maybe this condition self.data == other.data should not be part of that method.
No description provided.