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 type annotations to the PauliSum class #1104

Merged
merged 4 commits into from
Nov 20, 2019
Merged

Conversation

rht
Copy link
Contributor

@rht rht commented Nov 14, 2019

Description

Continuation of the effort to type-annotate paulis.py

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.
  • (New Feature) The docs have been updated accordingly.
  • (Bugfix) The associated issue is referenced above using
    auto-close keywords.
  • The changelog is updated,
    including author and PR number (@username, gh-xxx).

@rht
Copy link
Contributor Author

rht commented Nov 14, 2019

Here comes the conflict between Number and Union[int, float, complex] again:
E.g. pyquil/paulis.py:1028: error: Unsupported operand types for * ("float" and "PauliTerm") at

param_prog = exponential_map(coeff * first_pauli_term)
. This is because __rmul__'s argument is typed to be Number instead of Union[int, float, complex].

pyquil/paulis.py Outdated Show resolved Hide resolved
pyquil/paulis.py Outdated Show resolved Hide resolved
pyquil/paulis.py Outdated Show resolved Hide resolved
pyquil/paulis.py Outdated Show resolved Hide resolved
pyquil/paulis.py Outdated Show resolved Hide resolved
pyquil/paulis.py Show resolved Hide resolved
pyquil/paulis.py Outdated Show resolved Hide resolved
pyquil/paulis.py Outdated Show resolved Hide resolved
pyquil/paulis.py Show resolved Hide resolved
pyquil/paulis.py Show resolved Hide resolved
@karalekas karalekas added the quality 🎨 Improve code quality. label Nov 17, 2019
@karalekas karalekas added this to the v2.14 milestone Nov 17, 2019
@rht rht force-pushed the mypy branch 2 times, most recently from b09bd3a to 7761a5b Compare November 17, 2019 17:44
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.

This looks basically good to me. I think the only open question is whether we want to stick with Number for now or switch to Union[int, float, ...] everywhere.

pyquil/paulis.py Show resolved Hide resolved
pyquil/paulis.py Outdated Show resolved Hide resolved
pyquil/paulis.py Outdated Show resolved Hide resolved
pyquil/paulis.py Outdated Show resolved Hide resolved
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.

Sorry for all the back and forth, but this is what I think we should do (let's punt on the Number problem). If you make these two changes I'm happy to move forward 👍

pyquil/paulis.py Outdated
@@ -22,17 +22,20 @@
import numpy as np
import copy

from typing import Dict, FrozenSet, Iterable, Iterator, List, Optional, Tuple, Union
from typing import Callable, Dict, FrozenSet, Iterable, Iterator, List, Optional, Sequence, Tuple, Union
from pyquil.quilatom import ExpressionValueDesignator
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of importing this, let's define PauliCoefficientDesignator = Union[int, float, complex] and use that in place of ExpressionValueDesignator

pyquil/paulis.py Outdated
import warnings

PauliDesignator = Union['PauliTerm', 'PauliSum']
PauliT = Union[PauliDesignator, ExpressionValueDesignator]
Copy link
Contributor

Choose a reason for hiding this comment

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

let's remove PauliT and replace all of its uses with Union[PauliDesignator, PauliCoefficientDesignator]

@rht
Copy link
Contributor Author

rht commented Nov 19, 2019

@karalekas added the final change in the 3rd commit that will be squashed later.

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.

Two small changes. Otherwise lgtm.

pyquil/paulis.py Outdated Show resolved Hide resolved
pyquil/paulis.py Outdated Show resolved Hide resolved
@rht
Copy link
Contributor Author

rht commented Nov 20, 2019

@appleby addressed in commit "Third-round-of-review".

pyquil/paulis.py Outdated Show resolved Hide resolved
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.

looks good to me. thanks @rht!

@rht
Copy link
Contributor Author

rht commented Nov 20, 2019

I will squash after an approval from @karalekas.

@karalekas
Copy link
Contributor

LGTM! I can squash as part of the merge 👍

@karalekas karalekas merged commit 2f7e4b4 into rigetti:master Nov 20, 2019
@rht rht deleted the mypy branch November 20, 2019 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
quality 🎨 Improve code quality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants