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

Remove qubits from TomographyExperiment #107

Merged
merged 4 commits into from
May 1, 2019
Merged

Remove qubits from TomographyExperiment #107

merged 4 commits into from
May 1, 2019

Conversation

msohaibalam
Copy link
Contributor

Necessary update after merging of rigetti/pyquil#896

@msohaibalam msohaibalam requested a review from a team April 30, 2019 19:40
@msohaibalam
Copy link
Contributor Author

@astaley @jonward-rigetti @kylegulshen New PR for removal of qubits from TomographyExperiment

@msohaibalam msohaibalam force-pushed the remove-qubits-from-tomo-expt branch from d9027d6 to ca70d51 Compare April 30, 2019 19:45
@blakejohnson
Copy link
Contributor

Per the discussion in rigetti/pyquil#896, I would argue the correct solution is to add the qubits argument back order to not break semver expectations.

blakejohnson
blakejohnson reviewed Apr 30, 2019
View reviewed changes
requirements.txt
@@ -1,4 +1,4 @@
pyquil>=2.4.0
pyquil>=2.7.0
Copy link
Contributor

Choose a reason for hiding this comment

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

This change must imply a major version bump. We need to either revert the change to pyquil and re-release, or re-label this release as 3.0.0.

blakejohnson
blakejohnson suggested changes Apr 30, 2019
View reviewed changes
Copy link
Contributor

@blakejohnson blakejohnson left a comment

Choose a reason for hiding this comment

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

Pyquil versioning needs to be sorted out before proceeding.

@blakejohnson
Copy link
Contributor

I'm ok proceeding here now that we have a plan in place to provide a deprecation path on the prior API in pyquil.

@msohaibalam
Copy link
Contributor Author

Sounds good, merging this in now.

@msohaibalam msohaibalam merged commit 0e43222 into master May 1, 2019
@msohaibalam msohaibalam deleted the remove-qubits-from-tomo-expt branch May 1, 2019 16:10
@msohaibalam msohaibalam mentioned this pull request May 1, 2019
Closed
@blakejohnson
Copy link
Contributor

@msohaibalam: @astaley found that acquire_dfe_data still makes reference to the qubits attribute of TomographyExperiment. This points out that we haven't quite achieved full backwards compatibility in pyquil 2.7.1. We still need to populate the qubits attribute with the value that it would have had previously.

@msohaibalam
Copy link
Contributor Author

I see this particular line: https://github.com/rigetti/forest-benchmarking/blob/master/forest/benchmarking/direct_fidelity_estimation.py#L324.

Would it work if we replaced expr.qubits there with the qubits appearing in the estimated PauliTerm? From the description of estimate_dfe, it sounds like dimension is referring to the dimension of the Hilbert space that the measured PauliTerm is acting on.

blakejohnson
blakejohnson reviewed May 2, 2019
View reviewed changes
forest/benchmarking/direct_fidelity_estimation.py
cal_point_est=np.array([r.calibration_expectation for r in res]),
cal_std_err=np.array([r.calibration_stddev for r in res]),
cal_std_err=np.array([r.calibration_std_err for r in res]),
dimension=2**len(expr.qubits),
qubits=expr.qubits)
Copy link
Contributor

Choose a reason for hiding this comment

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

These two lines refer to expr.qubits.

@blakejohnson
Copy link
Contributor

Would it work if we replaced expr.qubits there with the qubits appearing in the estimated PauliTerm? From the description of estimate_dfe, it sounds like dimension is referring to the dimension of the Hilbert space that the measured PauliTerm is acting on.

Yes, that would work. We also need to populate the qubits field of DFEData.

@msohaibalam msohaibalam mentioned this pull request May 2, 2019
Merged
@msohaibalam
Copy link
Contributor Author

@blakejohnson Thanks for the input! I'm working on PR #115 to fix this bug asap. I'll ping you there once I'm sure the tests pass.

@karalekas karalekas added this to the v0.4 milestone May 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blakejohnson blakejohnson blakejohnson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.4
Development

Successfully merging this pull request may close these issues.
3 participants
@msohaibalam @blakejohnson @karalekas