Skip to content
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

Document content of the standard library #517

Merged
merged 10 commits into from
Apr 17, 2024
Merged

Conversation

jakelishman
Copy link
Contributor

@jakelishman jakelishman commented Mar 15, 2024

Summary

This describes the effects of the stdgates.inc include file as part of the language specification. I deliberately did not include the examples/stdgates.inc version of it in the specification to avoid implications that it must be implemented with that manner, but attempted to explain how implementations must behave as if if were defined like this, so that implementations are free to interpret the file in terms of defcal statements or the like (if they were to so choose).

This commit also includes a brief Sphinx extension, to make it possible to document OpenQASM gates in the full Sphinx structure. edit: now split off into https://github.com/openqasm/openqasm-sphinx and the openqasm-sphinx Python distribution.

Details and comments

Close #516.

The text here is largely just a first draft - happy for any suggestions to the requirements, or different ways of presenting things.

It probably makes sense for me to split of the Sphinx extension into a separately installable Python package, but just for the purposes of demoing this PR, I've included it inline here. If we want it, I'll sort out the legal stuff for my work on it on the IBM side (since here, it's just folded into the licensing for this repo) and move it into a separate repo. The current implementation is bare bones and just enough to document what I needed to. edit: Done as of 2024-04-09.

This describes the effects of the `stdgates.inc` include file as part of
the language specification.  I deliberately did not include the
`examples/stdgates.inc` version of it in the specification to avoid
implications that it *must* be implemented with that manner, but
attempted to explain how implementations must behave *as if* if were
defined like this, so that implementations are free to interpret the
file in terms of `defcal` statements or the like (if they were to so
choose).

This commit also includes a brief Sphinx extension, to make it possible
to document OpenQASM gates in the full Sphinx structure.
Copy link
Contributor

@hodgestar hodgestar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good to me.

source/language/standard_library.rst Outdated Show resolved Hide resolved
the rest of the program under.

Implementations must not define gates that are not present in chosen OpenQASM 3 version, where
doing so could cause a user program to be ill formed (for example due to a user attempting to define
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't any other gate definition potentially (silently) conflict with a user's gate definition?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention was to allow "implementation magic" to make extension gates available iff a user didn't later define them, such as to make later gates from later library versions available in an older program as a convenience mode - since the language syntax and library version are linked, it's plausible that somebody might want to write a program for older syntax but use a later library version, and accept that they're tied to a particular implementation. That can't be spelled in OpenQASM 3 in user programs, but implementations can / often do magic in the standard library.

I'm not precious about this proviso - I'm fine to drop it, since it would rely on implementation extensions anyway.

jlapeyre added a commit to jlapeyre/qiskit-core that referenced this pull request Mar 26, 2024
The biggest change in openqasm3_parser is in handling stdgates.inc.  Upon encountering
`include "stdgates.inc" openqasm3_parser reads no file, but rather adds symbols to the
symbol table for gates in the standard library.

The function `convert_asg` in the importer calls oq3_semantics::symbols::SymbolTable.gates
which returns a `Vec` of information about each "declared" gate. The information is the
gate's name and signature and SymbolID, which is sufficient to do everything the importer
could do before this commit.

Encountering `Stmt::GateDefinition` now is a no-op rather than an error.  (This was
previously called `Stmt::GateDeclaration`, but importantly it is really a definition.)

How the standard library, and gates in general, are handled will continue to evolve.

A behavioral difference between this commit and its parent: Before if the importer
encountered a gate call before the corresponding definition an error would be raised. With
the current commit, the importer behaves as if all gate definitions were moved to the top
of the OQ3 program. However, the error will still be found by the parser so that the
importer never will begin processing statements.

The importer depends on a particular branch of a copy of openqasm3_parser. When
the commit is ready to merge, we will release a version of openqasm3_parser and
depend instead on that release.

See openqasm/openqasm#517
jlapeyre added a commit to jlapeyre/qiskit-core that referenced this pull request Mar 26, 2024
This commit adds no new features or capabilities to the importer. But it brings
the importer up to date with the latest version of openqasm3_parser as a preliminary
step in improving the importer.

The biggest change in openqasm3_parser is in handling stdgates.inc.  Upon encountering
`include "stdgates.inc" openqasm3_parser reads no file, but rather adds symbols to the
symbol table for gates in the standard library.

The function `convert_asg` in the importer calls oq3_semantics::symbols::SymbolTable.gates
which returns a `Vec` of information about each "declared" gate. The information is the
gate's name and signature and SymbolID, which is sufficient to do everything the importer
could do before this commit.

Encountering `Stmt::GateDefinition` now is a no-op rather than an error.  (This was
previously called `Stmt::GateDeclaration`, but importantly it is really a definition.)

How the standard library, and gates in general, are handled will continue to evolve.

A behavioral difference between this commit and its parent: Before if the importer
encountered a gate call before the corresponding definition an error would be raised. With
the current commit, the importer behaves as if all gate definitions were moved to the top
of the OQ3 program. However, the error will still be found by the parser so that the
importer never will begin processing statements.

The importer depends on a particular branch of a copy of openqasm3_parser. When
the commit is ready to merge, we will release a version of openqasm3_parser and
depend instead on that release.

See openqasm/openqasm#517
jlapeyre added a commit to jlapeyre/qiskit-core that referenced this pull request Mar 26, 2024
This commit adds no new features or capabilities to the importer. But it brings
the importer up to date with the latest version of openqasm3_parser as a preliminary
step in improving the importer.

The biggest change in openqasm3_parser is in handling stdgates.inc.  Upon encountering
`include "stdgates.inc" openqasm3_parser reads no file, but rather adds symbols to the
symbol table for gates in the standard library.

The function `convert_asg` in the importer calls oq3_semantics::symbols::SymbolTable.gates
which returns a `Vec` of information about each "declared" gate. The information is the
gate's name and signature and SymbolID, which is sufficient to do everything the importer
could do before this commit.

Encountering `Stmt::GateDefinition` now is a no-op rather than an error.  (This was
previously called `Stmt::GateDeclaration`, but importantly it is really a definition.)

How the standard library, and gates in general, are handled will continue to evolve.

A behavioral difference between this commit and its parent: Before if the importer
encountered a gate call before the corresponding definition an error would be raised. With
the current commit, the importer behaves as if all gate definitions were moved to the top
of the OQ3 program. However, the error will still be found by the parser so that the
importer never will begin processing statements.

The importer depends on a particular branch of a copy of openqasm3_parser. When
the commit is ready to merge, we will release a version of openqasm3_parser and
depend instead on that release.

See openqasm/openqasm#517
@hodgestar
Copy link
Contributor

@jakelishman I'm done with my re-review. Apologies for the delay

jakelishman and others added 2 commits April 10, 2024 11:06
Co-authored-by: Simon Cross <hodgestar+github@gmail.com>
@jakelishman
Copy link
Contributor Author

Simon: I've pushed up 42fd895 that includes formal mathematics (deliberately avoiding the complications arising from a matrix representation) and symbol definitions for all the maps, so it's unambiguous what the mathematical symbols mean. Let me know if there's more you want.

hodgestar
hodgestar previously approved these changes Apr 10, 2024
Copy link
Contributor

@hodgestar hodgestar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

levbishop
levbishop previously approved these changes Apr 17, 2024
\texttt{crz(θ) a, b;} \mathit{CRZ}_{ba}(\theta)\colon\ \left\{\begin{alignedat}[c]2
{\lvert00\rangle}_{ba} &{}\to{}& &{\lvert00\rangle}_{ba}\\
{\lvert01\rangle}_{ba} &{}\to{}& e^{-i\theta/2} &{\lvert01\rangle}_{ba}\\
{\lvert10\rangle}_{ba} &{}\to{}& &{\lvert10\rangle}_{ba} \vphantom{e^{i\theta/2}}\\
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The \vphantom confuses github's rst viewer. But I'm ok with it so long as it looks right in the rendered file

Copy link
Contributor Author

@jakelishman jakelishman Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MathJax handles it fine. I was just being silly about the line spacings all coming out looking the same...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2024-04-17 at 16 25 40

Sample rendering

Copy link
Contributor Author

@jakelishman jakelishman Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and that picture just made me realise there's a typo in the inlined version of the CRX action. Fixed in 0b74092.

@jakelishman jakelishman dismissed stale reviews from levbishop and hodgestar via 0b74092 April 17, 2024 15:27
@levbishop levbishop merged commit 2c33cdd into openqasm:main Apr 17, 2024
11 checks passed
@jakelishman jakelishman deleted the stdlib branch April 17, 2024 15:50
github-merge-queue bot pushed a commit to Qiskit/qiskit that referenced this pull request May 3, 2024
…12087)

* Adapt crates/qasm3 to work with recent versions of openqasm3_parser

This commit adds no new features or capabilities to the importer. But it brings
the importer up to date with the latest version of openqasm3_parser as a preliminary
step in improving the importer.

The biggest change in openqasm3_parser is in handling stdgates.inc.  Upon encountering
`include "stdgates.inc" openqasm3_parser reads no file, but rather adds symbols to the
symbol table for gates in the standard library.

The function `convert_asg` in the importer calls oq3_semantics::symbols::SymbolTable.gates
which returns a `Vec` of information about each "declared" gate. The information is the
gate's name and signature and SymbolID, which is sufficient to do everything the importer
could do before this commit.

Encountering `Stmt::GateDefinition` now is a no-op rather than an error.  (This was
previously called `Stmt::GateDeclaration`, but importantly it is really a definition.)

How the standard library, and gates in general, are handled will continue to evolve.

A behavioral difference between this commit and its parent: Before if the importer
encountered a gate call before the corresponding definition an error would be raised. With
the current commit, the importer behaves as if all gate definitions were moved to the top
of the OQ3 program. However, the error will still be found by the parser so that the
importer never will begin processing statements.

The importer depends on a particular branch of a copy of openqasm3_parser. When
the commit is ready to merge, we will release a version of openqasm3_parser and
depend instead on that release.

See openqasm/openqasm#517

* Remove unnecessary conversion `name.to_string()`

Response to reviewer comment https://github.com/Qiskit/qiskit/pull/12087/files#r1586949717

* Remove check for U gate in map_gate_ids

* This requires modifying the external parser crate.
* Depend temporarily on the branch of the parser with this modification.
* Make another change required by other upstream improvments.
* Cargo config files are changed because of above changes.

* Return error returned by map_gate_ids in convert_asg

Previously this error was ignored. This would almost certainly cause
an error later. But better to do it at the correct place.

* Remove superfluous call to `iter()`

* Depend on published oq3_semantics 0.6.0

* Fix cargo lock file

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
ElePT pushed a commit to ElePT/qiskit that referenced this pull request May 31, 2024
…iskit#12087)

* Adapt crates/qasm3 to work with recent versions of openqasm3_parser

This commit adds no new features or capabilities to the importer. But it brings
the importer up to date with the latest version of openqasm3_parser as a preliminary
step in improving the importer.

The biggest change in openqasm3_parser is in handling stdgates.inc.  Upon encountering
`include "stdgates.inc" openqasm3_parser reads no file, but rather adds symbols to the
symbol table for gates in the standard library.

The function `convert_asg` in the importer calls oq3_semantics::symbols::SymbolTable.gates
which returns a `Vec` of information about each "declared" gate. The information is the
gate's name and signature and SymbolID, which is sufficient to do everything the importer
could do before this commit.

Encountering `Stmt::GateDefinition` now is a no-op rather than an error.  (This was
previously called `Stmt::GateDeclaration`, but importantly it is really a definition.)

How the standard library, and gates in general, are handled will continue to evolve.

A behavioral difference between this commit and its parent: Before if the importer
encountered a gate call before the corresponding definition an error would be raised. With
the current commit, the importer behaves as if all gate definitions were moved to the top
of the OQ3 program. However, the error will still be found by the parser so that the
importer never will begin processing statements.

The importer depends on a particular branch of a copy of openqasm3_parser. When
the commit is ready to merge, we will release a version of openqasm3_parser and
depend instead on that release.

See openqasm/openqasm#517

* Remove unnecessary conversion `name.to_string()`

Response to reviewer comment https://github.com/Qiskit/qiskit/pull/12087/files#r1586949717

* Remove check for U gate in map_gate_ids

* This requires modifying the external parser crate.
* Depend temporarily on the branch of the parser with this modification.
* Make another change required by other upstream improvments.
* Cargo config files are changed because of above changes.

* Return error returned by map_gate_ids in convert_asg

Previously this error was ignored. This would almost certainly cause
an error later. But better to do it at the correct place.

* Remove superfluous call to `iter()`

* Depend on published oq3_semantics 0.6.0

* Fix cargo lock file

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include specification of stdgates.inc
6 participants