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

Add type hints to gates and QuantumCircuit #6831

Merged
merged 9 commits into from
Aug 13, 2021

Conversation

jakelishman
Copy link
Member

@jakelishman jakelishman commented Jul 29, 2021

Summary

This is pretty much all documentation changes, except that it also moves the definition of QuantumCircuit.measure into quantumcircuit.py rather than monkey-patching it from measure.py. This makes it consistent with other instruction definitions, and made it easier to access the QubitSpecifier and ClbitSpecifier type aliases.

Add type hints to QuantumCircuit

This adds full type hints to all public function entry points in
QuantumCircuit. This will not cause quantumcircuit.py to pass a mypy
check yet; there are various idioms used within the file which are not
statically type-safe (nor, in some cases, dynamically type safe in the
sense that we sometimes rely on mismatched types to throw exceptions).
These include anything which takes the inplace argument, since
inplace=True versions do not return, which influences the out types to
Optional["QuantumCircuit"]
This then means that methods using these functions (e.g.
QuantumCircuit.__add__) become unsafe; even though they use the
default inplace=False, subclasses could override this while still
maintaining type safety, and so the operation is not safe.

This patch makes no attempt to modify the behaviour of these
non-type-safe functions, only to classify what the current types are.

Add type hints and label to all standard gate classes

This completes the standard-library gate documentation overhaul tracked
by #3705, and mostly done in #4017, by adding type hints to the
initializers.

It also adds the missing label option to the iSWAP, R, RXX, RYY, RZX
and RZZ gates, to bring them up to parity with the other gates, tracked
in #6780.

Add parameters to QuantumCircuit gate docstrings

This only adds short documentation to the docstrings of the gate methods
(for example QuantumCircuit.x) about the parameters and the return
values. It does not currently import the matrix representations into
the docstrings themselves, because the gate classes include those. The
expectation is that you would see the docstring when running
interactively, and even in Jupyter, this typically does not include
displaying maths. The matrix form is more useful when looking at the
online Sphinx documentation, where the cross-reference gate link will
work more easily.

Details and comments

Fix #3705
Fix #6780

I didn't pull the actual matrix definitions into the QuantumCircuit docstrings, because I think they'll add a lot of unreadable noise to the points where you'd actually see them most commonly - in the Jupyter/IDE popups for help and completions. These are text-based, and so don't really show maths in the most part. The only places you expect to see the maths are in the Spinx websites, where the links that are already in the docstring will work anyway.

There is a slight nuisance at the moment in that neither sphinx.ext.napoleon nor sphinx_autodoc_typehints quite handle cross-referencing the typehints in the docstrings properly, but this is something we could fix externally.

There are a few parts in QuantumCircuit where we're not doing things fully type-safely, and a couple of places where we do it in such a way that mypy can't statically analysise it. I haven't attempted to fix any of this, since that can be done elsewhere.

This adds full type hints to all public function entry points in
QuantumCircuit.  This will not cause quantumcircuit.py to pass a mypy
check yet; there are various idioms used within the file which are not
statically type-safe (nor, in some cases, dynamically type safe in the
sense that we sometimes rely on mismatched types to throw exceptions).
These include anything which takes the `inplace` argument, since
`inplace=True` versions do not return, which influences the out types to
    Optional["QuantumCircuit"]
This then means that methods using these functions (e.g.
`QuantumCircuit.__add__`) become unsafe; even though they use the
default `inplace=False`, subclasses could override this while still
maintaining type safety, and so the operation is not safe.

This patch makes no attempt to modify the behaviour of these
non-type-safe functions, only to classify what the current types are.
This completes the standard-library gate documentation overhaul tracked
by Qiskit#3705, and mostly done in Qiskit#4017, by adding type hints to the
initializers.

It also adds the missing `label` option to the iSWAP, R, RXX, RYY, RZX
and RZZ gates, to bring them up to parity with the other gates, tracked
in Qiskit#6780.
This only adds short documentation to the docstrings of the gate methods
(for example `QuantumCircuit.x`) about the parameters and the return
values.  It does not currently import the matrix representations into
the docstrings themselves, because the gate classes include those.  The
expectation is that you would see the docstring when running
interactively, and even in Jupyter, this typically does not include
displaying maths.  The matrix form is more useful when looking at the
online Sphinx documentation, where the cross-reference gate link will
work more easily.
@jakelishman jakelishman requested a review from a team as a code owner July 29, 2021 13:52
@jakelishman
Copy link
Member Author

Oh yeah, in other details: this leaves the fractured nature of the label parameter in the gate methods of QuantumCircuit, because we internally haven't made a 100% decision one way or the other. We can make those changes in the future if we need to.

Python 3.6 doesn't allow assigning to the __doc__ field of the `typing`
types, so as long as we support it, we can't do that.
@levbishop
Copy link
Member

Looks good. Any reason you didn't bring reset in like you did with measure?
For a future PR: the consensus seems to be settling on qiskit.extensions being a mistake so perhaps one day we could eventually eliminate monkeypatching completely...

levbishop
levbishop previously approved these changes Aug 13, 2021
The previous commit stopped `QuantumCircuit.measure` from being
monkey-patched in by `qiskit/circuit/measure.py`, but the similar
`QuantumCircuit.reset` operation was overlooked.  This adds it in for
consistency.
@jakelishman
Copy link
Member Author

@levbishop: no good reason - I suspect I noticed that measure was being monkey-patched in because it's referenced within QuantumCircuit anyway, and I just didn't notice that reset wasn't it. I've brought it in as well.

There are still 3 monkey-patched functions in the circuit library (QuantumCircuit.mcr[xyz]), but these seem to be of quite a different form so I've left them as-is. Aer also patches a bunch of functionality in, so it's not like we can be entirely consistent just within Terra. At any rate, this inclusion is pretty tangential to the rest of this PR - I think I only did the measure originally because mypy moaned about it.

@jakelishman
Copy link
Member Author

jakelishman commented Aug 13, 2021

Oh, I should highlight: the hints I've added should all be correct (I believe), but there's a few places where the inplace argument messes with mypy's static analysis. Things like the arithmetic operators use functions which technically return Optional[T] (because of the inplace argument), but since we use them with inplace=False, we know they do return T. Depending on how serious we want to be about the type hinting, I can add typing.cast calls to sort them out in those cases.

@levbishop
Copy link
Member

For now lets just make any added hints correct, since that is uncontroversial.
Clogging up the logic with extra "useless" code such as casting, just to appease mypy should probably wait for if we ever decide to make mypy a part of our CI strategy.

@jakelishman jakelishman added automerge Changelog: API Change Include in the "Changed" section of the changelog labels Aug 13, 2021
@mergify mergify bot merged commit 2cd42b4 into Qiskit:main Aug 13, 2021
@jakelishman jakelishman deleted the gate-type-hints branch August 13, 2021 15:24
@kdk kdk added this to the 0.19 milestone Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: API Change Include in the "Changed" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistency in whether gates accept labels Gate docstrings and type hints
3 participants