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

Snapshot circuit extensions #317

Merged
merged 16 commits into from
Aug 14, 2019
Merged

Snapshot circuit extensions #317

merged 16 commits into from
Aug 14, 2019

Conversation

kanejess
Copy link
Contributor

@kanejess kanejess commented Aug 2, 2019

Summary

Getting started on #300 - i.e. improving usability of different snapshot types.

Details and comments

Subclasses and methods written for SnapshotStatevector #301, SnapshotStabilizer #302, SnapshotDensityMatrix #303, SnapshotProbabilities #304, and SnapshotExpectationValue #305.

So far, tests are only checking that the snapshots have been created, and do not check that snapshot data is correct.

Still needs fixing:

  • 'density_matrix' test gives error 'ERROR: Circuit contains invalid instructions ( invalid snapshot instructions: {density_matrix})'

test

test

test

test

test

test

test

test

test

test

test

test

test
typo in test

test

test fixes

uncommenting important things

test

test

test

tidying up

renaming files; testing density_matrix snapshot

renamed file

renamed file

small typo fix

fixed import in __init__.py
@chriseclectic chriseclectic added this to the 0.3 milestone Aug 2, 2019
@chriseclectic
Copy link
Member

@kanejess To match terra I would suggest making a separate file for each type of snapshot and placing them inside aer/extensions/snapshot.py, aer/extensions/snapshot_probabilities.py etc, and then having an __init__.py inside the extensions folder that imports each one

Copy link
Contributor

@maddy-tod maddy-tod 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 good so far, I have a few comments/questions inline.

qiskit/providers/aer/aer_snapshots.py Outdated Show resolved Hide resolved
qiskit/providers/aer/aer_snapshots.py Outdated Show resolved Hide resolved
qiskit/providers/aer/aer_snapshots.py Outdated Show resolved Hide resolved
qiskit/providers/aer/extensions/snapshot.py Show resolved Hide resolved
@atilag
Copy link
Member

atilag commented Aug 13, 2019

Hi @kanejess , what is left to remove the [WIP]?. We targeted this to be included in 0.3 release which will happen very soon.

Copy link
Contributor

@maddy-tod maddy-tod 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 better! Once you have address these changes I think we can remove the [WIP]. Also can you take a look at the linting, as that has failed on Travis.

qiskit/providers/aer/extensions/snapshot.py Outdated Show resolved Hide resolved
qiskit/providers/aer/extensions/snapshot.py Show resolved Hide resolved
qiskit/providers/aer/extensions/snapshot_density_matrix.py Outdated Show resolved Hide resolved
@atilag
Copy link
Member

atilag commented Aug 13, 2019

There's a lot of code repetition in this PR, and other things that could be simplified, but that would probably require a deep redesign on how snapshots are implemented so I'm not going to postpone it as far as it works.
We can have the redesign discussion after 0.3 release.

@atilag
Copy link
Member

atilag commented Aug 13, 2019

I'd need confirmation from @kanejess for tomorrow to include this in the release.
Otherwise will be postponed to 0.3.1.

@kanejess kanejess changed the title [WIP] Snapshot circuit extensions Snapshot circuit extensions Aug 13, 2019
maddy-tod
maddy-tod previously approved these changes Aug 14, 2019
@atilag atilag merged commit 4d17ad2 into Qiskit:master Aug 14, 2019
dcmckayibm pushed a commit to dcmckayibm/qiskit-aer that referenced this pull request Nov 3, 2019
* Adding SnapshotStatevector and tests
* Adding StabilizerSnapshot and tests
* Adding SnapshotDensityMatrix and tests
* Adding SnapshotExpectationValue and tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants