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 missing equivalences for XX{Plus,Minus}YYGate #9017

Merged
merged 3 commits into from
Oct 28, 2022

Conversation

jakelishman
Copy link
Member

Summary

These are just copied from the _define method on the classes. The equivalence-library tests are updated (since subTest doesn't isolate correctly with testtools) to print out all the names of the tests being run, so it's clear that the existing test now tests these definitions.

Details and comments

Bonus points to anyone who already knew the esoteric Python scoping rules that forbade me from using a list comprehension to define the test parametrisation here.

These are just copied from the `_define` method on the classes.  The
equivalence-library tests are updated (since `subTest` doesn't isolate
correctly with `testtools`) to print out all the names of the tests
being run, so it's clear that the existing test now tests these
definitions.
@jakelishman jakelishman added the Changelog: New Feature Include in the "Added" section of the changelog label Oct 27, 2022
@jakelishman jakelishman requested a review from a team as a code owner October 27, 2022 20:50
@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

@coveralls
Copy link

coveralls commented Oct 27, 2022

Pull Request Test Coverage Report for Build 3345143334

  • 16 of 16 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 84.492%

Totals Coverage Status
Change from base Build 3343035335: 0.01%
Covered Lines: 62175
Relevant Lines: 73587

💛 - Coveralls

Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻 Would it make sense to automatically add the default decomposition of gates into the equivalence library instead of copy+pasting the actual definition? Or maybe the other way around: <SomeStandardGate>._define invokes the equivalence library and runs that (but maybe this way around it's too slow...)

@jakelishman
Copy link
Member Author

I think (but am speaking purely from my own views) that the general direction we're heading in is to have the equivalence library be the primary source of definitions for gates, at least, and _define just be a user convenience for custom gates. I haven't thought enough about that yet - for now, copy-paste is probably enough.

(This gate would already have been fine in transpile I think, because it would have been unrolled before hitting the 1q optimiser that caused a test failure in a linked PR.)

@jakelishman jakelishman added this to the 0.23.0 milestone Oct 28, 2022
@mergify mergify bot merged commit b45afea into Qiskit:main Oct 28, 2022
@jakelishman jakelishman deleted the xxyy-equivalence branch October 30, 2022 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants