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.

Sign up for GitHub

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

Jump to bottom

Fix qpy for MCX gates #9391

Merged
merged 5 commits into from
Jan 20, 2023
Merged

Fix qpy for MCX gates #9391

merged 5 commits into from
Jan 20, 2023

Conversation

ElePT
Copy link
Contributor

@ElePT ElePT commented Jan 19, 2023 •
edited
Loading

Summary

This PR fixes #9390.

Details and comments

@ElePT ElePT requested a review from a team as a code owner January 19, 2023 11:00
@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:

@ElePT ElePT mentioned this pull request Jan 19, 2023
Closed
@coveralls
Copy link

coveralls commented Jan 19, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 3971521387

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 84.924%
Totals Coverage Status
Change from base Build 3971520636: 0.0%
Covered Lines: 66705
Relevant Lines: 78547
💛 - Coveralls

jakelishman
jakelishman reviewed Jan 19, 2023
View reviewed changes
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

This fix looks fine to me, thanks. You can add a use of this gate into this existing test: https://github.com/Qiskit/qiskit-terra/blob/a0466b36c7086166f780747d501b0fbcdc2863cd/test/python/circuit/test_circuit_load_from_qpy.py#L1044

The tests of QPY are in test/python/circuit because that's where QPY itself used to live, before it got its own subpackage. In the same file there are also some larger tests of things like QFT - if there's an intermediate algorithm library object for IPE, you could add a similar test to the QFT one too.

This wants a bugfix release note too.

@ElePT ElePT changed the title Fix qpy for `MCXGrayCode gate Fix qpy for MCXGrayCode gate Jan 19, 2023
usesnames
usesnames previously approved these changes Jan 19, 2023
View reviewed changes
@mtreinish mtreinish added stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: Bugfix Include in the "Fixed" section of the changelog labels Jan 19, 2023
jakelishman
jakelishman previously approved these changes Jan 19, 2023
View reviewed changes
Copy link
Member

@jakelishman jakelishman 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, but we'll wait until everything else for 0.23.0 is merged before we merge this, to avoid holding up the release with CI.

This can get merged between rc1 and the full release, since it's a pure bugfix.

@ElePT
Copy link
Contributor Author

ElePT commented Jan 19, 2023

Thanks @jakelishman!! That sounds good.

@jakelishman jakelishman added the on hold Can not fix yet label Jan 19, 2023
mtreinish
mtreinish reviewed Jan 19, 2023
View reviewed changes
qiskit/qpy/binary_io/circuits.py Outdated
@@ -252,7 +252,7 @@ def _read_instruction(file_obj, circuit, registers, custom_operations, version,
if gate_name in {"IfElseOp", "WhileLoopOp"}:
gate = gate_class(condition_tuple, *params)
elif version >= 5 and issubclass(gate_class, ControlledGate):
if gate_name in {"MCPhaseGate", "MCU1Gate"}:
if gate_name in {"MCPhaseGate", "MCU1Gate", "MCXGrayCode"}:
Copy link
Member

Choose a reason for hiding this comment

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

I'm just curious if there are other gates in the mcx family that have the same constructor and need this. I always get lost in all the mcx variants. But it might be good to check we've got all of them covered here

Copy link
Contributor Author

@ElePT ElePT Jan 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You were right @mtreinish, qpy also fails for other MCX gates. Namely (I think these are all):

MCXRecursive
MCXVChain
MCXGate

I can add these to the PR. I though I had tested it before realizing that I was using 2 control qubits, so the gate ended up being of type CCXGate (of course my checks passed).

@mtreinish mtreinish removed the on hold Can not fix yet label Jan 19, 2023
jakelishman
jakelishman previously approved these changes Jan 20, 2023
View reviewed changes
@ElePT ElePT changed the title Fix qpy for MCXGrayCode gate Fix qpy for MCX gates Jan 20, 2023
@mtreinish
Copy link
Member

Oh, I was 2min too slow on my comments just ignore those suggestions, decreasing the control qubit counts on the gates works just as well

@mergify mergify bot merged commit ad95294 into Qiskit:main Jan 20, 2023
@mergify mergify bot mentioned this pull request Jan 20, 2023
Merged
mergify bot pushed a commit that referenced this pull request Jan 20, 2023
@ElePT @mergify
Fix qpy for MCX gates (#9391)
53e25fa 
* Add MCXGrayCode

* Add test and reno

* Add other mcx gates

* Fix test

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit ad95294)
mergify bot added a commit that referenced this pull request Jan 21, 2023
@mergify @ElePT
Fix qpy for MCX gates (#9391) (#9412)
ddb8c29 
* Add MCXGrayCode

* Add test and reno

* Add other mcx gates

* Fix test

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit ad95294)

Co-authored-by: ElePT <57907331+ElePT@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
This was referenced Feb 3, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtreinish mtreinish mtreinish left review comments

@jakelishman jakelishman jakelishman approved these changes

@usesnames usesnames usesnames left review comments

Assignees
No one assigned
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

QPY fails to serialize MCXGrayCode gate
6 participants
@ElePT @qiskit-bot @coveralls @mtreinish @jakelishman @usesnames