[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.
| @@ -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: |
There was a problem hiding this comment.
It absolutely is. I don't know why, I used getattr at some point in one of my first issues for accessing the instructions, and it just stuck to me 😆
| ] | ||
|
|
||
| @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)) |
There was a problem hiding this comment.
Thanks for this suggestion! Looks much nicer indeed.
| 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).
There was a problem hiding this comment.
In that case (of course, correct me if I am wrong), should we remove the test_map_method altogether? I am suggesting this since in test_controlled_gates we test the functionality of circuit.map() (for the MipMapper) rather thoroughly, with both circuits initialised from a string, and with the CircuitBuilder, for scenarios in which circuit remapping is needed and in which it is not.
| 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.
There was a problem hiding this comment.
Indeed this is no longer needed, so i removed the .items() from the assertions, in both test_simple_mappers.py and test_mip_mapper.py. Regarding the condition in Mapping.__eq__ what should be done about it? Should this be part of a different issue?
No description provided.