🐛 Support `u` gate when parsing OpenQASM 2

cda-tum / mqt-core

MQT Core - The Backbone of the Munich Quantum Toolkit

http://mqt.readthedocs.io/projects/core

MIT License

52 stars 28 forks source link

🐛 Support `u` gate when parsing OpenQASM 2 #639

Closed ystade closed 2 months ago

ystade commented 2 months ago

Description

🚧 Should probably have opened an issue but I wanted to directly add the failing test.

What I did

I downloaded a circuit for AE with 2 qubits compiled hardware-agnostically with qiskit from MQT Bench and inserted this circuit as input to qc::QuantumComputation::fromQASM(), which should not throw an error but it does, see the respective test https://github.com/cda-tum/mqt-core/blob/f91a0526617dbb68e2ce3589a8994f220bc44347/test/test_operation.cpp#L263-L287

Checklist:

[x] The pull request only contains commits that are related to it.
[x] I have added appropriate tests and documentation.
[x] I have made sure that all CI jobs on GitHub pass.
[x] The pull request introduces no new warnings and follows the project's style guidelines.

ystade commented 2 months ago

Also, I am not sure whether the place for the new test is appropriate, feel free to suggest something different.

burgholzer commented 2 months ago

This was a pretty quick fix. Although, technically, this is not even a bug on our side, but on the Qiskit side. A well known one in fact. The Qiskit OpenQASM 2.0 exporter is known to hallucinate the existence of the u gate in the standard library, while, in fact, the gate is not part of the standardized qelib1.inc, but was only part of Qiskit's extension of said file.

So, technically, our parser is right to reject the circuit since it is not valid OpenQASM 2.0. However, many people have come to rely on these circuits to be readable and Qiskit itself has implemented a respective workaround for reading the files.

Given that it's a one line change, I think it should be fine to also adopt the workaround here.

burgholzer commented 2 months ago

Also, I am not sure whether the place for the new test is appropriate, feel free to suggest something different.

I moved the test to the OpenQASM parser test suite and extended it slightly.

Also adjusted the labels of this PR. The "bug" label is typically reserved for issues (=bug reports), while the "fix" label is intended for PRs (=bug fixes). And given that this is only a bug fix and not a breaking change of any sort nor a new feature, it qualifies as a patch release contribution and does not really warrant a new minor version. Hope that makes sense 😌

ystade commented 2 months ago

@burgholzer Thanks for taking care of this!