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

Fix wrong argument supplied to _identity_op() #9201

Merged
merged 6 commits into from
Mar 1, 2023

Conversation

mstnb
Copy link
Contributor

@mstnb mstnb commented Nov 24, 2022

Summary

This fixes a typo in the CommutationChecker where 2**num_qubits was passed to _identity_op instead of num_qubits.

Details and comments

Fixes #9197.

@mstnb mstnb requested a review from a team as a code owner November 24, 2022 18:49
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Nov 24, 2022
@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:

  • @Qiskit/terra-core

@CLAassistant
Copy link

CLAassistant commented Nov 24, 2022

CLA assistant check
All committers have signed the CLA.

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 looks correct, thanks for the quick response! Please could you also add a test in the file test/python/circuit/test_commutation_checker.py that we get a correct result - something like

from qiskit.circuit import CommutationChecker, Qubit
from qiskit.circuit.library import XGate

qargs = [Qubit() for _ in [None]*8]
CommutationChecker().commute(XGate(), qargs[:1], [], XGate().control(7), qargs, [])

should be False, for example, and will completely blow up Numpy without your fix. You could write a test about that.

We also need to add a short release note - if you've done pip install reno already, you should be able to run reno new --edit fix-commutation-checker-size and remove everything but the fixes section (and write a sentence there briefly explaining what was fixed).

@jakelishman jakelishman 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 Nov 24, 2022
@jakelishman jakelishman added this to the 0.22.4 milestone Nov 24, 2022
@jakelishman jakelishman changed the title Fix wrong argument supplied to _identity_op() qiskit/qiskit-terra#9197 Fix wrong argument supplied to _identity_op() Nov 24, 2022
@coveralls
Copy link

coveralls commented Nov 24, 2022

Pull Request Test Coverage Report for Build 4307516998

  • 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 85.343%

Totals Coverage Status
Change from base Build 4307516345: 0.0%
Covered Lines: 67530
Relevant Lines: 79128

💛 - Coveralls

@mstnb
Copy link
Contributor Author

mstnb commented Nov 25, 2022

Okay, is there any naming convention for these test cases? And I guess all the methods in the QiskitTestCase are called automatically so I can simply add a new method?

@jakelishman
Copy link
Member

Sorry for the delay in responding here. As long as the method name starts with test_ (and doesn't clash with another), yeah, it gets called automatically - QiskitTestCase is a subclass of Python's built-in unittest.TestCase, so all the assertion methods on those classes should work as well.

@mstnb
Copy link
Contributor Author

mstnb commented Dec 25, 2022

Hi, I didn't get to work on this any sooner. I now added a simple unit test and release note. I wanted to merge the upstream changes to my repo to be able to push my changes. Apparently I pressed the wrong button now, sorry for that.
I messed something up on my local repo, trying to get that fixed.
I think I merged upstream to the wrong branch, now I have a detached head on my main branch. Haha can you help me out there so I don't break it even more?

@jakelishman
Copy link
Member

A "detached HEAD" state in git means you've just got a commit checked out that isn't the end point of any branch. If you've not got any changes in your working directory, you can just git checkout <branch> where <branch> is wherever you want to get back to.

If you've got the changes you want to push in your local copy right now, but git is saying weird things, then you might want to try copying the files with changes in to somewhere safe, then running

git fetch origin
git checkout bug-9197-fix
git merge origin/bug-9197-fix

Ideally this will just update your local branch with the changes that have happened to the remote on GitHub, and then you'll just be able to run git push to update GitHub with the changes you've made locally. The point of copying the files is just so you don't lose your changes if something goes wrong.

Don't worry - there's nothing you can do that can break our side of things, and the worst that can possibly happen is that your branch gets too messed up, and then we can close this and you can just make a new pull request.

@jakelishman jakelishman removed this from the 0.22.4 milestone Jan 13, 2023
mtreinish and others added 3 commits March 1, 2023 12:31
This commit adds a test case to ensure we get the correct result with a
large number of qubits. This also implicitly tests that we've fixed the
excessive memory consumption because the memory requirements for an 8
qubit gate with the bug still present would not be runnable on most
current systems.

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
@mtreinish
Copy link
Member

To unblock this since the same bug being fixed by this was reported again in #9693 I pushed up the test proposed by @jakelishman and also added a release note.

@mergify mergify bot merged commit 992d74f into Qiskit:main Mar 1, 2023
mergify bot pushed a commit that referenced this pull request Mar 1, 2023
* Fix wrong argument supplied to _identity_op() #9197

* Add test case with large qubit gate

This commit adds a test case to ensure we get the correct result with a
large number of qubits. This also implicitly tests that we've fixed the
excessive memory consumption because the memory requirements for an 8
qubit gate with the bug still present would not be runnable on most
current systems.

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>

* Add release note

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 992d74f)
mergify bot added a commit that referenced this pull request Mar 1, 2023
* Fix wrong argument supplied to _identity_op() #9197

* Add test case with large qubit gate

This commit adds a test case to ensure we get the correct result with a
large number of qubits. This also implicitly tests that we've fixed the
excessive memory consumption because the memory requirements for an 8
qubit gate with the bug still present would not be runnable on most
current systems.

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>

* Add release note

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 992d74f)

Co-authored-by: M St <25572273+mstnb@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
king-p3nguin pushed a commit to king-p3nguin/qiskit-terra that referenced this pull request May 22, 2023
* Fix wrong argument supplied to _identity_op() Qiskit#9197

* Add test case with large qubit gate

This commit adds a test case to ensure we get the correct result with a
large number of qubits. This also implicitly tests that we've fixed the
excessive memory consumption because the memory requirements for an 8
qubit gate with the bug still present would not be runnable on most
current systems.

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>

* Add release note

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog Community PR PRs from contributors that are not 'members' of the Qiskit repo stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
Status: Done
6 participants