✨ Introducing Qiskit's Target as our main Device representation and Remove OQC Lucy #560
Conversation
nquetschlich
added
enhancement
# Conflicts: # src/mqt/bench/benchmark_generation.py # src/mqt/bench/devices/__init__.py # src/mqt/bench/devices/device.py # src/mqt/bench/output.py
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
burgholzer
added
major
There was a problem hiding this comment.
Thanks @nquetschlich for spearheading this 🙏🏼
I have not made it through the whole PR yet, but I think I got enough of an idea to suggest targeted (pun intended) improvements.
I hope the inline comments roughly make sense and give you some direction on where this could/should be heading.
| gateset: Target | None = None, | ||
| device: Target | None = None, |
There was a problem hiding this comment.
There is no need to distinguish between these anymore.
It is totally sufficient to have a single target: Target | None = None parameter.
The native gates level will just extract the gateset from the target.
The mapped level will use the full target.
The other levels do not use the information at all.
| @@ -90,10 +90,10 @@ def generate_filename( | |||
| base = f"{benchmark_name}_{level}" | |||
|
|
|||
| if level == "nativegates" and gateset is not None and opt_level is not None: | |||
| return f"{base}_{gateset.name}_opt{opt_level}_{num_qubits}" | |||
| return f"{base}_{gateset.description}_opt{opt_level}_{num_qubits}" | |||
There was a problem hiding this comment.
I am fairly sure the description needs some kind of sanitization.
Many descriptions of targets in the wild will use spaces ("Hardware Provider XZY Target").
Based on the above comment, the gateset should be a serialized/stringified version of the gateset and the device description should be a sanitized version of the description.
There was a problem hiding this comment.
I added a simple sanitization.
| @@ -219,7 +219,7 @@ def get_indep_level( | |||
| @overload | |||
| def get_native_gates_level( | |||
| qc: QuantumCircuit, | |||
| gateset: Gateset, | |||
| gateset: Target, | |||
There was a problem hiding this comment.
I'd simply refer to this as target everywhere.
There was a problem hiding this comment.
Agreed and adjusted it. However, for the get_benchmark method I think it is more easily understandable to still call it gateset. I would like to discuss this and some other points in person.
| gates = {inst.name for inst, _ in gateset.instructions} | ||
| print(gates) |
There was a problem hiding this comment.
There is a print statement here, which I am pretty surprised that ruff did not complain about or optimize away. Maybe worth checking the config.
Other than that, we should simply be able to use *target.operation_names in the call down below.
There was a problem hiding this comment.
Thanks for catching that, your suggestions works well.
src/mqt/bench/devices/calibration.py
Outdated
There was a problem hiding this comment.
I would argue that this file can be removed as I believe there should be no need to carry all these calibration files with us here.
src/mqt/bench/devices/ionq.py
Outdated
| # Copyright (c) 2023 - 2025 Chair for Design Automation, TUM | ||
| # Copyright (c) 2025 Munich Quantum Software Company GmbH | ||
| # All rights reserved. | ||
| # | ||
| # SPDX-License-Identifier: MIT | ||
| # | ||
| # Licensed under the MIT License |
There was a problem hiding this comment.
What happened here?
| # Copyright (c) 2023 - 2025 Chair for Design Automation, TUM | |
| # Copyright (c) 2025 Munich Quantum Software Company GmbH | |
| # All rights reserved. | |
| # | |
| # SPDX-License-Identifier: MIT | |
| # | |
| # Licensed under the MIT License |
Please also fix in the other files.
src/mqt/bench/devices/ionq.py
Outdated
| with calibration_path.open() as json_file: | ||
| calib = json.load(json_file) | ||
|
|
||
| class IonQAria1(IonQDevice): | ||
| """IonQ Aria1 device.""" | ||
| target = Target(num_qubits=calib["num_qubits"], description=calib["name"]) | ||
| num_qubits = calib["num_qubits"] | ||
| connectivity = calib["connectivity"] | ||
|
|
||
| def __init__(self) -> None: | ||
| """Initialize the IonQ Aria1 device.""" | ||
| super().__init__(get_device_calibration_path("ionq_aria1")) | ||
| # Gate durations and fidelities | ||
| oneq_fidelity = calib["fidelity"]["1q"]["mean"] | ||
| twoq_fidelity = calib["fidelity"]["2q"]["mean"] | ||
| spam_fidelity = calib["fidelity"]["spam"]["mean"] | ||
|
|
||
| oneq_duration = calib["timing"]["1q"] | ||
| twoq_duration = calib["timing"]["2q"] | ||
| readout_duration = calib["timing"]["readout"] |
There was a problem hiding this comment.
I would be very tempted to simply hard code the calibration file values here.
We have never updated them. Since their creation. And they are fairly specific to a particular device.
We could simply link to the origin of the data.
Or, wild and crazy idea: Simply remove all the calibration information for now to streamline the implementation and then come up with a good strategy to selectively add it back wherever feasible.
MQT Bench was not using that information so far anyway.
There was a problem hiding this comment.
I like the idea and would like to discuss the consequences in terms of MQT Predictor in person. Let's then decide based on that discussion.
src/mqt/bench/output.py
Outdated
| if target: | ||
| gateset = sorted({str(inst.name) for inst, _ in target.instructions}) |
There was a problem hiding this comment.
| if target: | |
| gateset = sorted({str(inst.name) for inst, _ in target.instructions}) | |
| if target: | |
| gateset = target.operation_names |
or similar
There was a problem hiding this comment.
Thanks, followed that suggestion.
| lines.append(f"// Used gateset: {gateset}") | ||
| c_map = target.build_coupling_map() if target else None |
There was a problem hiding this comment.
Pretty sure this function will need an additional level input.
Because otherwise, this will add a coupling map even if a circuit is only transpiled to the native gates level
There was a problem hiding this comment.
I don't think so, since the build_coupling_map() functions returns None according to its docstring if there are no connectivity constraints present in the target (see https://github.com/Qiskit/qiskit/blob/ee4095933e688efb794e679b3feda658fefdec9b/qiskit/transpiler/target.py#L545).
# Conflicts: # src/mqt/bench/devices/devices.py
This PR introduces the Qiskit Target object as our new device representation (resolves issue #553).
Furthermore, the deprecated OQC Lucy device was removed.
Additonally, the
FakeBackendsare not used anymore to be compatible with Qiskit v2.