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 indexing and inner product to Statevector #7095

Merged
merged 40 commits into from
Oct 21, 2021

Conversation

DavideFrr
Copy link
Contributor

@DavideFrr DavideFrr commented Oct 4, 2021

Summary

Make Statevector objects subcriptables to fix #4916
Add inner product method to Statevector objects to fix #4934

Details and comments

@DavideFrr DavideFrr requested review from chriseclectic and a team as code owners October 4, 2021 07:24
@javabster
Copy link
Contributor

Hi @DavideFrr thanks for working on this! It looks like you've got an indentation issue that's causing the test failures and some linting (you can run tox -eblack for the linting). Could you also flesh out the PR description a bit more and add a release note please? For future reference if you put fixes #issue-number in the PR description Github will automatically link the issue to the PR which makes life easier for the reviewers 😄

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.

Hi there, thanks for looking at this. I've left a few comments inline in the text - there's a few of them, but most of them are very minor style tweaks. There's a couple that bear a little bit of thought, though, about how we should be handling negative keys, and inner products with items that aren't statevectors.

DavideFrr and others added 5 commits October 5, 2021 13:39
Co-authored-by: Jake Lishman <jake@binhbar.com>
…r-product_method-a0337393d9a5b666.yaml

Co-authored-by: Jake Lishman <jake@binhbar.com>
…r-product_method-a0337393d9a5b666.yaml

Co-authored-by: Jake Lishman <jake@binhbar.com>
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.

The "suggestions" feature inline is usually for code reviewers as opposed to the person making the PR: we suggest, and then you commit them if you agree. If you want to change the code, it's your code, so you can just push the commits! In these cases, since you're suggesting stuff that was exactly in line with my comments, you can just go ahead and commit them directly, and I can review again.

test/python/quantum_info/states/test_statevector.py Outdated Show resolved Hide resolved
qiskit/quantum_info/states/statevector.py Show resolved Hide resolved
qiskit/quantum_info/states/statevector.py Outdated Show resolved Hide resolved
@jakelishman jakelishman changed the title Statevector feature Add indexing and inner product to Statevector Oct 15, 2021
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.

Sorry it took me a little while to reply to this. The features look fine, and the tests of their working behaviour seem good, but please can we add some tests of the failure paths as well?

(When you've made a complete set of changes, if you press the "re-request review" button next to my name, it'll pop up my priority list.)

test/python/quantum_info/states/test_statevector.py Outdated Show resolved Hide resolved
@DavideFrr DavideFrr requested a review from jakelishman October 19, 2021 14:26
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.

Thanks for sticking with this, it looks good now!

@jakelishman jakelishman added automerge Changelog: New Feature Include in the "Added" section of the changelog mod: quantum info Related to the Quantum Info module (States & Operators) labels Oct 21, 2021
@jakelishman jakelishman added this to the 0.19 milestone Oct 21, 2021
@mergify mergify bot merged commit a53888d into Qiskit:main Oct 21, 2021
@DavideFrr DavideFrr deleted the statevector_feature branch October 21, 2021 22:12
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 mod: quantum info Related to the Quantum Info module (States & Operators)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow calculating inner products between Statevector objects Statevector objects are not scriptable
3 participants