Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add importer for OpenQASM 3 #9347
Add importer for OpenQASM 3 #9347
Changes from 1 commit
9184649
131909b
cc8548c
45f996a
531b7ff
e5b409a
66e3810
fc9931a
85e0f5a
baf9af6
f121fa4
95366fb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 do not have any high-level disagreement with these function names on their own. It is however worth pointing out that this API breaks from the current OpenQASM2 APIs - https://qiskit.org/documentation/stubs/qiskit.qasm.Qasm.html#qiskit.qasm.Qasm.
I wonder if it might make sense to begin renaming the older API as
qasm2
(with module deprecation paths) and to extend the QuantumCircuit methods for OpenQASM3 support?This could be follow-up work.
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.
While I was on holiday, I actually wrote a whole new OpenQASM 2 parser for Qiskit, with the majority in Rust. It was mostly for my own learning experience, but since it's like 10x faster than the current Qiskit one (with no external Python dependencies), I might make a PR changing it.
With or without though, though, I think we should move to a new
qasm2
module with the namesload
,loads
,dump
anddumps
. I'm not keen to add an extra method toQuantumCircuit
because the class is already massively overloaded - I'd rather we keep things in separate packages. Theload
/dump
terminology is the standard Python names (e.g.pickle.dump
,json.dump
, etc), and it's consistent with the currently existingqasm3.dump
andqpy.dump
as well - Qiskit is already semi-inconsistent, but really it's the old OQ2 handling that's inconsistent with the rest of the package direction.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.
Thanks, Jake, I'd be hesitant to add more work and potential breaks to the existing QASM2 parser for those that are using it given we're planning on migrating away from it but the pathway suggested for the qasm2 module seems good.
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.
Yeah, agree that there's no reason to break users' workflows. My rough intent was to keep
QuantumCircuit.from_qasm_{str,file}
, and just cause them to internally callqasm2.load
andqasm2.loads
as appropriate (assuming I do actually publish that parser and it's suitable for Terra).