-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 release note for Expr
support
#10503
Conversation
One or more of the the following people are requested to review this:
|
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.
Lgtm from a wording perspective :)
* :func:`~.expr.greater` | ||
* :func:`~.expr.greater_equal` | ||
|
||
These can act on Python integer and Boolean literals, or on :class:`.ClassicalRegister` |
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.
Nit, Python uses lowercase
These can act on Python integer and Boolean literals, or on :class:`.ClassicalRegister` | |
These can act on Python integer and boolean literals, or on :class:`.ClassicalRegister` |
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.
We capitalise most adjectives derived from proper nouns (Gaussian, Laplacian, Jacobian, etc), and Qiskit's mostly just inconsistent on "Boolean" in particular. I don't really know why we don't afford George Boole the same courtesy as others ;). At any rate, in all documentation I've written for Qiskit, I've capitalised it.
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.
Fwiw, even python.org is inconsistent on this: both forms of capitalisation are present on https://docs.python.org/3/library/stdtypes.html, say.
|
||
This is a feature that hardware is only just beginning to support, so Qiskit's support is | ||
starting conservatively, and you may well find rough edges in places where hardware doesn't | ||
support expressions that look simple in Qiskit. Please bear with us, and your hardware vendors! |
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.
"bear with us" is a weird English idiom. Maybe this to be more accessible to non-native speakers?
support expressions that look simple in Qiskit. Please bear with us, and your hardware vendors! | |
support expressions that look simple in Qiskit. Please be patient with us and your hardware vendors! |
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 can change it if you think it's much better, but "endure" is a common meaning of "bear" ("I can bear the pain" / "I couldn't bear the experience") - I wouldn't disagree that "bear with us" is idiomatic, but I would disagree that it's weird or unusual.
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.
Personally I would have worded this a bit more formally I don't take too much issue with the use of ":bear: with us" as an idiom (as long as we use the proper character :P ), but more I'd have said something like:
This feature is new for both Qiskit and the available quantum hardware that Qiskit works with. As the features are still being developed there are likely to be places where there are unexpected edge cases that will need some time to be worked out. If you encounter any issue around classical expression support or usage please open an issue with Qiskit or your hardware vendor.
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, that's probably better than what I wrote
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'm fine if you want to stick with bear
.
Pull Request Test Coverage Report for Build 5669229161
💛 - Coveralls |
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 had some comments inline about the separation of the release notes. None are critical or worth blocking over though because we can easily adjust it post-backport in #10497.
|
||
This is a feature that hardware is only just beginning to support, so Qiskit's support is | ||
starting conservatively, and you may well find rough edges in places where hardware doesn't | ||
support expressions that look simple in Qiskit. Please bear with us, and your hardware vendors! |
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.
Personally I would have worded this a bit more formally I don't take too much issue with the use of ":bear: with us" as an idiom (as long as we use the proper character :P ), but more I'd have said something like:
This feature is new for both Qiskit and the available quantum hardware that Qiskit works with. As the features are still being developed there are likely to be places where there are unexpected edge cases that will need some time to be worked out. If you encounter any issue around classical expression support or usage please open an issue with Qiskit or your hardware vendor.
from OpenQASM 3 is currently managed by `a separate package <https://github.com/Qiskit/qiskit-qasm3-import>`__ | ||
(which is re-exposed via :mod:`qiskit.qasm3`), which we hope will be extended to match the new | ||
features in Qiskit. | ||
- | |
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.
This reads like a continuation of the above release note should it really be in a separate note?
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 don't feel super strongly, but I approximately considered them separate parts of the feature. If you think it'll be better to combine all three, that's fine by me.
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 think it's fine to have it as a separate note, as I agree it's a separate feature. It was more just the wording here starts with "In addition" and the tone sounds like it's part of the previous note instead of describing just the expression visitor class and what it's used for and how it's used.
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.
That's fair. Want me to fix it here (since it's not in the mergeable part of the queue anyway) or wait til fixup later?
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.
Oh I missed the question here until now and it's the head of the merge queue. Lets just fix it as part of #10497 at this point.
respectively produce an iterator through the :class:`~.expr.Var` nodes and check whether two | ||
:class:`~.expr.Expr` instances are structurally the same, up to some mapping of the | ||
:class:`~.expr.Var` nodes contained. | ||
- | |
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.
Same question here the flow between these notes seem like they're just separate paragraphs of the same release note instead of separate bullet points in the features section. I know reno puts them all together since they're in the same file as function of how it generates the rst output. But, I always try to think of separate notes as if they order will be shuffled when they're assembled, and in this case I feel like this (and even more so with the above note) they wouldn't be able to stand on their own if they were shuffled.
* Bump version. * Add 0.25.0 release prelude. * Remove 0.24 backports from releasenotes/notes/0.25. * Update releasenotes/config.yaml for categories. * New features section. * Fix circular import. * Add misc section. * Fixes. * More fixes. * Fixes. * Hack for section containing only other subsections. * Upgrades section. * Deprecations section. * Bugfix section. * Update prelude. * Address review comments. * Update algorithms prelude. * Additional fixups. * Address review comments. * Update for comments on #10503. --------- Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Summary
Close #10331.
Details and comments