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 PauliTerm class #1075

Merged
merged 1 commit into from
Nov 14, 2019
Merged

Conversation

rht
Copy link
Contributor

@rht rht commented Oct 24, 2019

Description

Only PauliTerm is annotated to keep this PR small.

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 rht requested a review from a team as a code owner October 24, 2019 12:58
@rht
Copy link
Contributor Author

rht commented Oct 24, 2019

Based on #147.

Copy link
Contributor

@notmgsk notmgsk left a comment

Choose a reason for hiding this comment

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

Please complete the checklist (namely, fill out the changelog).

Paging the resident mypy expert @appleby.

@notmgsk notmgsk requested a review from appleby October 24, 2019 14:26
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 @rht, thanks for the PR. I noticed in the linked issue there were problems type checking numbers.Number. Is that still an issue?

Also, does mypy report any errors/warnings when running with --strict ? E.g.

mypy --ignore-missing-imports --follow-imports=silent --strict pyquil/paulis.py

pyquil/paulis.py Outdated Show resolved Hide resolved
@rht rht force-pushed the mypy branch 2 times, most recently from 846343d to a098b51 Compare October 24, 2019 15:01
@rht
Copy link
Contributor Author

rht commented Oct 24, 2019

I have added a description of the PR into the changelog.

@rht
Copy link
Contributor Author

rht commented Oct 24, 2019

Also, does mypy report any errors/warnings when running with --strict ? E.g.

There are lots of errors/warnings still because not everything inside paulis.py has been annotated yet. Hopefully the number of errors/warnings become tractable as more of the functions become annotated.

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.

Minor stuff. Otherwise looks good.

Github won't let me comment on lines that don't have diff, so mentioning here: there appear to be a handful of PauliTerm methods that are missing return type hints. (I suspect these methods were added after your original PR.) Would you mind adding type hints for them? E.g.

  • operations_as_set
  • __hash__
  • program
  • __iter__

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
@rht
Copy link
Contributor Author

rht commented Oct 24, 2019

@appleby I have applied your patches to the PR. I will add annotations to the remaining 4 methods, and I will report when I'm done.

@rht
Copy link
Contributor Author

rht commented Oct 24, 2019

@appleby I added operations_as_set, __hash__, program, __iter__ annotations in the second commit.

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 more small changes; otherwise looks good.

pyquil/paulis.py Outdated Show resolved Hide resolved
pyquil/paulis.py Show resolved Hide resolved
pyquil/paulis.py Show resolved Hide resolved
@rht rht force-pushed the mypy branch 2 times, most recently from f0d95f6 to 54c6b7e Compare October 24, 2019 20:55
@rht
Copy link
Contributor Author

rht commented Oct 24, 2019

The second commit has been squashed.

@rht
Copy link
Contributor Author

rht commented Oct 24, 2019

There is an inconsistency with PauliTerm's __init__() where the coefficient argument should have been a Number instead of Union[int, float, complex]. But I want to keep this PR compact. I will do a subsequent PR that addresses these issues later.

@appleby
Copy link
Contributor

appleby commented Oct 24, 2019

There is an inconsistency with PauliTerm's __init__() where the coefficient argument should have been a Number instead of Union[int, float, complex]. But I want to keep this PR compact. I will do a subsequent PR that addresses these issues later.

Given this mypy issue that you pointed out in #147, which seems to imply that numbers.Number is somewhat useless for type checking, maybe we should go the other route and switch all the Numbers to Union[int, float, complex] (as unsatisfying as that would be).

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.

There are still some issues to work out, mostly around the arithmetic operations, but I think that is an issue of mypy needing more help to correctly infer types. The types added in this PR look reasonable, so I feel OK taking a gradual approach and adding more type hints to the module in a follow on PR.

@rht
Copy link
Contributor Author

rht commented Oct 26, 2019

Given this mypy issue that you pointed out in #147, which seems to imply that numbers.Number is somewhat useless for type checking, maybe we should go the other route and switch all the Numbers to Union[int, float, complex] (as unsatisfying as that would be).

There is an extra complication due to the fact that the code uses isinstance(x, Number) which works, since a complex object is an instance of Number. Using different convention for type hints (i.e. Union[int, float, complex]) and type checking in code would cause conflict.

@rht
Copy link
Contributor Author

rht commented Oct 26, 2019

I already have PauliSum annotation done. Just waiting for this PR to be merged, then I will make a new PR of PauliSum.

@karalekas
Copy link
Contributor

I will give this a review as soon as I've gotten @appleby's typing PR over the line -- thanks for doing this! :)

@rht
Copy link
Contributor Author

rht commented Nov 9, 2019

I just rebased to get rid of the merge conflict. @karalekas any update?

@karalekas karalekas added this to the v2.14 milestone Nov 14, 2019
@karalekas karalekas added the quality 🎨 Improve code quality. label Nov 14, 2019
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.

@rht this looks great, and I'm looking forward to the follow-on PR. only thing that needs changing is moving your changelog entry to the new v2.14 "Improvements and Changes" section, which will require you to rebase off of master first

@karalekas karalekas changed the title Mypy: Annotate PauliTerm Add type annotations to the PauliTerm class Nov 14, 2019
@rht
Copy link
Contributor Author

rht commented Nov 14, 2019

@karalekas rebased and moved the line to be under v2.14.

@karalekas karalekas self-requested a review November 14, 2019 17:44
@karalekas karalekas merged commit 7b8cc72 into rigetti:master Nov 14, 2019
@karalekas
Copy link
Contributor

@rht thanks for contributing to pyQuil! :)

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