-
Notifications
You must be signed in to change notification settings - Fork 4
[CQT-421] Implement QGymMapper mapper pass #642
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
[CQT-421] Implement QGymMapper mapper pass #642
Conversation
juanboschero
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff
| @@ -0,0 +1,69 @@ | |||
| The [Qgym](https://github.com/QuTech-Delft/qgym) package functions in a manner similar | |||
| to the well known gym package, in the sense that it provides a number of environments | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| to the well known gym package, in the sense that it provides a number of environments | |
| to the well known gym package, in the sense that it provides a number of environments |
| well as a `TRPO.zip` file containing the weights of a trained agent in your working | ||
| directory. | ||
|
|
||
| ```python |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example below should be in the test_docs file
| return interaction_graph | ||
|
|
||
| def _get_mapping(self, mapping_data: Any, qubit_register_size: int) -> Mapping: | ||
| """Convert QGym's physical-to-logical mapping to OpenSquirrel's logical-to-physical mapping.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing args and return in docstring
| mapping_data = self._extract_mapping_data(last_obs) | ||
| return self._get_mapping(mapping_data, qubit_register_size) | ||
|
|
||
| def _ir_to_interaction_graph(self, ir: IR) -> nx.Graph: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docstring args + return
elenbaasc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for the effort 😄!
Some small refactors required, but no major changes of functionality (I think)
| - [Hardcoded Mapper](hardcoded-mapper.md) (`HardcodedMapper`) | ||
| - [Identity Mapper](identity-mapper.md) (`IdentitiyMapper`) | ||
| - [Random Mapper](random-mapper.md) (`RandomMapper`) | ||
| - [Qgym Mapper](qgym-mapper.md) (`QGymMapper`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it Qgym or QGym? We should be consistent. If it is Qgym, then we should also call the pass the QgymMapper, or leave it as is if it is QGym, but then adjust the text.
| @@ -0,0 +1,69 @@ | |||
| The [Qgym](https://github.com/QuTech-Delft/qgym) package functions in a manner similar | |||
| to the well known gym package, in the sense that it provides a number of environments | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| to the well known gym package, in the sense that it provides a number of environments | |
| to the well-known gym package, in the sense that it provides a number of environments |
| @@ -0,0 +1,69 @@ | |||
| The [Qgym](https://github.com/QuTech-Delft/qgym) package functions in a manner similar | |||
| to the well known gym package, in the sense that it provides a number of environments | |||
| on which reinforcement learning (RL) agents can be applied. The main purpose of qgym is | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New sentences are always on new lines. (change here and in other places)
| on which reinforcement learning (RL) agents can be applied. The main purpose of qgym is | |
| on which reinforcement learning (RL) agents can be applied. | |
| The main purpose of qgym is |
| with open('connectivities.json', 'r') as file: | ||
| connectivities = json.load(file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this needs to be a JSON, can you not simply provide a connectivity (dict[str, list[int]]) as we defined it for example for the routers?
| connection_graph = nx.Graph() | ||
| connection_graph.add_edges_from(hardware_connectivity) | ||
|
|
||
| qgym_mapper = QGymMapper(agent_class = "TRPO", agent_path = "TRPO.zip", | ||
| connection_graph=connection_graph) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather pass the connectivity and then define what a connection_graph is in in the QgymMapper.
| assert str(circuit1) != initial_circuit | ||
|
|
||
|
|
||
| def test_check_not_many_logical_as_physical_qubits(mapper1: QGymMapper, circuit2: Circuit) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| def test_check_not_many_logical_as_physical_qubits(mapper1: QGymMapper, circuit2: Circuit) -> None: | |
| def test_unequal_number_logical_and_physical_qubits(mapper1: QGymMapper, circuit2: Circuit) -> None: |
|
|
||
| @pytest.mark.parametrize( | ||
| "mapper, circuit, expected_mapping_length", # noqa: PT006 | ||
| [("mapper1", "circuit1", 5), ("mapper2", "circuit2", 7)], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| [("mapper1", "circuit1", 5), ("mapper2", "circuit2", 7)], | |
| [ | |
| ("mapper1", "circuit1", 5), | |
| ("mapper2", "circuit2", 7) | |
| ], |
| @pytest.mark.parametrize( | ||
| "mapper, circuit, expected_mapping_length", # noqa: PT006 | ||
| [("mapper1", "circuit1", 5), ("mapper2", "circuit2", 7)], | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ) | |
| ids=["tuna-5-mapping", "starmon-7-mapping"], | |
| ) |
| mapper = request.getfixturevalue(mapper) # type: ignore[arg-type] | ||
| mapping = mapper.map(circuit.ir, circuit.qubit_register_size) | ||
| assert isinstance(mapping, Mapping) | ||
| assert len(mapping) == expected_mapping_length |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we test more than just the length? Or will the resultant mapping be different each time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should put these (*.zips) and the qgym_mapper.py module in a separate folder qgym_mapper.
No description provided.