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

adding copy argument to QFTGate.__array__ #12979

Merged
merged 1 commit into from
Aug 19, 2024

Conversation

alexanderivrii
Copy link
Contributor

Summary

Fixes numpy support for QFTGate: the QFT gate PR was developed in parallel with #11999 and did not get the required upgrade.

@alexanderivrii alexanderivrii added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Aug 19, 2024
@alexanderivrii alexanderivrii requested a review from a team as a code owner August 19, 2024 09:01
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Cryoris
  • @Qiskit/terra-core
  • @ajavadia

Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

Thanks a lot @alexanderivrii! I added a tiny suggestion but the PR can also be merged as-is, given that this bug is becoming annoying in the merge queue.

"""Return a numpy array for the QFTGate."""
if copy is False:
raise ValueError("unable to avoid copy while creating an array as requested")
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny tiny suggestion:

Suggested change
raise ValueError("unable to avoid copy while creating an array as requested")
raise ValueError("Unable to avoid copy while creating an array as requested")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @ElePT! I agree that the capitalized "U" looks better, but all the other 30+ identical message use lower-case "u".

Copy link
Contributor

Choose a reason for hiding this comment

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

hah, tunnel vision. Let's keep it consistent then.

@ElePT
Copy link
Contributor

ElePT commented Aug 19, 2024

The QFT gate was added in 1.2, right? (#11463) In that case we should also backport the fix.

@ElePT ElePT added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Aug 19, 2024
@ElePT ElePT enabled auto-merge August 19, 2024 09:23
@coveralls
Copy link

Pull Request Test Coverage Report for Build 10450381480

Details

  • 2 of 3 (66.67%) changed or added relevant lines in 1 file are covered.
  • 4 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.03%) to 89.594%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/circuit/library/basis_change/qft.py 2 3 66.67%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 4 91.73%
Totals Coverage Status
Change from base Build 10423065630: 0.03%
Covered Lines: 67603
Relevant Lines: 75455

💛 - Coveralls

@ElePT ElePT added this pull request to the merge queue Aug 19, 2024
@ElePT ElePT removed this pull request from the merge queue due to a manual request Aug 19, 2024
@ElePT ElePT added this pull request to the merge queue Aug 19, 2024
Merged via the queue into Qiskit:main with commit 06392c5 Aug 19, 2024
15 checks passed
mergify bot pushed a commit that referenced this pull request Aug 19, 2024
github-merge-queue bot pushed a commit that referenced this pull request Aug 20, 2024
(cherry picked from commit 06392c5)

Co-authored-by: Alexander Ivrii <[email protected]>
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 stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants