Skip to content

🐛 Support u gate when parsing OpenQASM 2 #639

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

Merged
merged 3 commits into from
Jul 1, 2024
Merged

Conversation

ystade
Copy link
Collaborator

@ystade ystade commented Jul 1, 2024

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:

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

@ystade ystade added bug Something isn't working minor Minor version update c++ Anything related to C++ code labels Jul 1, 2024
@ystade ystade self-assigned this Jul 1, 2024
@ystade
Copy link
Collaborator Author

ystade commented Jul 1, 2024

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

@burgholzer burgholzer added fix Fix for something that isn't working Core Anything related to the Core library and IR and removed bug Something isn't working minor Minor version update labels Jul 1, 2024
@burgholzer burgholzer marked this pull request as ready for review July 1, 2024 09:18
@burgholzer
Copy link
Member

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
Copy link
Member

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 😌

@burgholzer burgholzer changed the title 🐛 Fix Parsing of MQT-Bench Circuits 🐛 Support u gate when parsing OpenQASM 2 Jul 1, 2024
@burgholzer burgholzer merged commit b8b0c8e into main Jul 1, 2024
34 checks passed
@burgholzer burgholzer deleted the fix-bench-input branch July 1, 2024 09:28
@ystade
Copy link
Collaborator Author

ystade commented Jul 1, 2024

@burgholzer Thanks for taking care of this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Anything related to C++ code Core Anything related to the Core library and IR fix Fix for something that isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants