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

Remove qubits as an input from TomographyExperiment #896

Merged
merged 3 commits into from
Apr 29, 2019

Conversation

msohaibalam
Copy link
Contributor

The qubits field is a redundant input to TomographyExperiment, in the sense that measure_observables never uses it for anything at all. This PR removes this extraneous input.

@msohaibalam msohaibalam requested a review from a team as a code owner April 26, 2019 18:28
@karalekas karalekas merged commit 70c3b72 into rigetti:master Apr 29, 2019
@msohaibalam msohaibalam deleted the remove-qubits-from-tomo-expt branch April 29, 2019 18:07
@karalekas karalekas added this to the v2.7.0 milestone Apr 29, 2019
@jonward-rigetti
Copy link

jonward-rigetti commented Apr 30, 2019

🌴 wasn't ready for this change @msohaibalam @karalekas

@msohaibalam
Copy link
Contributor Author

msohaibalam commented Apr 30, 2019

@jonward-rigetti I can create a PR for 🌴to reflect these changes very soon. Is there anywhere else you can think of where pyquil: operator_estimation is used? The only other place I'm aware of is rigetti/forest-benchmarking

@jonward-rigetti
Copy link

jonward-rigetti commented Apr 30, 2019

I'll take care of the 🌴 -side fix, but this does make me wonder about how to avoid this in the future.

@notmgsk
Copy link
Contributor

notmgsk commented Apr 30, 2019

Test internal tools against pyQuil master?

@msohaibalam
Copy link
Contributor Author

I'll take care of the willow-side fix, but this does make me wonder about how to avoid this in the future.

I think the best way is to create simultaneous PRs. There will be a few more changes coming very soon to pyquil: operator_estimation. I can create the relevant corresponding PRs in forest-benchmarking and can tag you so you know to create the appropriate change in willow (or I can take care of that too, either way)

@jonward-rigetti
Copy link

That sounds reasonable, but we should also have a CI solution, e.g. triggering a new 🌴 master build when pyquil master gets updated.

@notmgsk
Copy link
Contributor

notmgsk commented Apr 30, 2019

Should probably do the same for 🐦 etc

@caryan
Copy link
Contributor

caryan commented Apr 30, 2019

We could also not make breaking API changes on a point release. We could have kwarg'd qubits to default to None and if it was passed in logged a warning saying we're that on a future release we will no longer use this parameter.

@jonward-rigetti
Copy link

Yes, we need to follow a deprecation path for signature changes.

@blakejohnson
Copy link

I will echo @caryan's comment. This change should trigger a major version bump in pyquil since it modifies a public API.

@msohaibalam
Copy link
Contributor Author

msohaibalam commented Apr 30, 2019

@blakejohnson @caryan @astaley @karalekas This and other changes are necessary before we can more widely advertise this part of the API, and it starts getting used by external users. There was an internal discussion on the pros and cons of changing parts of pyQuil: operator_estimation, and the conclusion was to make the necessary changes, create docs, examples etc. as regular PRs in pyQuil, and create appropriate updates to any libraries that we internally use. If this conclusion needs to be revisited, I'm all ears. But I don't think it's a good idea to not make the changes that we had decided to make, or go through several hoops to make it "backwards compatible", since the work on pyquil: operator_estimation has not yet finished. Could it be a good compromise to pin the version(s) of forest-benchmarking and 🌴 to older versions of pyquil, and bump their versions up, and create the necessary updates, once the work on pyquil: operator_estimation is complete?

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.

6 participants