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

Call qc.run_symmetrized_readout in measure_observables #1047

Merged
merged 14 commits into from
Oct 29, 2019

Conversation

kylegulshen
Copy link
Contributor

@kylegulshen kylegulshen commented Oct 10, 2019

Description

closes #1046. Note that we are still supporting the deprecated readout_symmetrize argument; I'm not sure if this is an appropriate opportunity to drop it?

Checklist

  • The above description motivates these changes.
  • There is a unit test that covers these changes.
  • All new and existing tests pass locally and on Semaphore.
  • Parameters have type hints with PEP 484 syntax.
  • Functions and classes have useful sphinx-style docstrings.
  • The changelog is updated,
    including author and PR number (@username, gh-xxx).

@kylegulshen kylegulshen requested a review from a team as a code owner October 10, 2019 17:52
Copy link
Contributor

@appleby appleby left a comment

Choose a reason for hiding this comment

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

more minor docstring stuff

pyquil/operator_estimation.py Outdated Show resolved Hide resolved
pyquil/operator_estimation.py Outdated Show resolved Hide resolved
pyquil/operator_estimation.py Outdated Show resolved Hide resolved
kylegulshen and others added 2 commits October 17, 2019 11:08
Co-Authored-By: appleby <86076+appleby@users.noreply.github.com>
pyquil/operator_estimation.py Outdated Show resolved Hide resolved
pyquil/operator_estimation.py Show resolved Hide resolved
@karalekas
Copy link
Contributor

Also changelog needs to be updated etc.

Copy link
Contributor

@karalekas karalekas left a comment

Choose a reason for hiding this comment

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

Here's a little snippet that should hopefully explain what I'm thinking!

import warnings
from enum import IntEnum

class SymmetrizationLevel(IntEnum):
    EXHAUSTIVE = -1
    NONE = 0
    ONE = 1
    TWO = 2
    THREE = 3

def fn(level: int = SymmetrizationLevel.EXHAUSTIVE):
    if level is None:
        warnings.warn(f'Providing None to symmetrize_readout is deprecated, setting to {SymmetrizationLevel.NONE}')
        level = SymmetrizationLevel.NONE
    elif isinstance(level, str):
        warnings.warn(f'Providing a string to symmetrize_readout is deprecated, setting to {SymmetrizationLevel.EXHAUSTIVE}')
        level = SymmetrizationLevel.EXHAUSTIVE
    elif level not in list(SymmetrizationLevel):
        raise TypeError(f'The symmetrize_readout argument must be one of the following ints {list(SymmetrizationLevel)}')
    return level
In [2]: fn(4)
---------------------------------------------------------------------------
Exception                                 Traceback (most recent call last)
<ipython-input-2-90297181a897> in <module>
----> 1 fn(4)

<ipython-input-1-5b7d718a32db> in fn(level)
     17         level = SymmetrizationLevel.EXHAUSTIVE
     18     elif level not in list(SymmetrizationLevel):
---> 19         raise Exception(f'The symmetrize_readout argument must be one of the following ints {list(SymmetrizationLevel)}')
     20     return level
     21

Exception: The symmetrize_readout argument must be one of the following ints [<SymmetrizationLevel.EXHAUSTIVE: -1>, <SymmetrizationLevel.NONE: 0>, <SymmetrizationLevel.ONE: 1>, <SymmetrizationLevel.TWO: 2>, <SymmetrizationLevel.THREE: 3>]

pyquil/operator_estimation.py Outdated Show resolved Hide resolved
pyquil/operator_estimation.py Outdated Show resolved Hide resolved
pyquil/operator_estimation.py Outdated Show resolved Hide resolved
pyquil/operator_estimation.py Outdated Show resolved Hide resolved
symmetrize_readout = SymmetrizationLevel.NONE
warnings.warn("'symmetrize_readout' should now be an int, 0 instead of none.",
DeprecationWarning)
elif symmetrize_readout == 'exhaustive':
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if someone passes a string that is not "exhaustive"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it matches previous behavior, you just get an error that tells you which values are supported.

pyquil/operator_estimation.py Outdated Show resolved Hide resolved
pyquil/operator_estimation.py Outdated Show resolved Hide resolved
pyquil/operator_estimation.py Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
kylegulshen and others added 2 commits October 21, 2019 12:29
Co-Authored-By: Peter Karalekas <peter@rigetti.com>
Copy link
Contributor

@karalekas karalekas left a comment

Choose a reason for hiding this comment

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

Last batch of nitpicks.

pyquil/operator_estimation.py Outdated Show resolved Hide resolved
pyquil/operator_estimation.py Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@karalekas karalekas changed the title Use QPU symmetrization in place of old operator estimation symmetrization. Call qc.run_symmetrized_readout within measure_observables Oct 28, 2019
@karalekas karalekas changed the title Call qc.run_symmetrized_readout within measure_observables Call qc.run_symmetrized_readout in measure_observables Oct 28, 2019
@karalekas karalekas added the refactor 🔨 Rework existing functionality. label Oct 28, 2019
@karalekas karalekas added this to the v2.13 milestone Oct 28, 2019
Co-Authored-By: Peter Karalekas <peter@rigetti.com>
@karalekas karalekas merged commit d7876e4 into rigetti:master Oct 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor 🔨 Rework existing functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use QPU symmetrization in place of old operator estimation symmetrization.
3 participants