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

Pauli string parsing #1292

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

dwillmer
Copy link

@dwillmer dwillmer commented Dec 31, 2020

First attempt at fixing #1007, with potential fixes/benefits for #1008

  • The initial requirement was to allow for Pauli strings that contain only operators, eg. X0, however this functionality and some of the other requests (ie. Improve error messaging around Pauli string parsing #1008) were also symptoms of the fact that regex was being used, rather than a fully-fledged parser.
  • The lark library was already being used elsewhere in this project and was in the requirements file, so I've replaced the regexp usage with a custom lark parser (defined as a short python string rather than a separate file, just for brevity) to take care of converting the Pauli strings to the correct python objects.
  • This has wider benefits, and has allowed the use of shorthand such as 0.7X1 to represent 0.7 * X1, which was one of the 'expected fail' unit-tests previously (now moved to 'expected pass'), as well as allowing just X0 / X1 as Pauli short strings (also moved from expected fail to expected pass).
  • The from_custom_str method for PauliTerm has been entirely replaced by the new parser, but I stopped short of replacing the from_custom_str on PauliSum, because the API currently expects a list of PauliTerm objects to be summed; this behaviour is now usable with the new parsing code, but it would be a wider-ranging API change to use the parser instead of the one remaining regex in PauliSum, because of the change from a list of terms to a single term, which I decided was not an appropriate decision to make in this PR.
  • There's an interesting nuance to the parser in that we want to support 0.5X1 (ie. coefficient first, operator second), but we don't want to support X10.5 (ie. coefficient second) because the operator has a numeric value as the last digit, so this is obviously ambiguous. As a result the lark parser has a slightly larger than expected ?pauli_term: section, rather than supporting all permutations of operators and coefficients up front.
  • This also includes a full suite of unit-tests for the new parser and usage for PauliTerm/PauliSum classes, as well as checking expected error messages for bad inputs.

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 [Travis CI][travis].
  • Parameters and return values have type hints with [PEP 484 syntax][pep-484].
  • Functions and classes have useful [Sphinx-style][sphinx] docstrings.
  • All code follows [Black][black] style and obeys [flake8][flake8] conventions.
  • (New Feature) The [docs][docs] have been updated accordingly.
  • (Bugfix) The associated issue is referenced above using [auto-close keywords][auto-close].
  • The [changelog][changelog] is updated, including author and PR number (@username, gh-xxx).

@dwillmer dwillmer requested a review from a team as a code owner December 31, 2020 13:46
@dwillmer dwillmer marked this pull request as draft December 31, 2020 13:49
@notmgsk
Copy link
Contributor

notmgsk commented Dec 31, 2020

Nice work, @dwillmer! I'll give this a proper look over next week :)

@dwillmer dwillmer marked this pull request as ready for review June 1, 2021 22:14
@ameyer-rigetti
Copy link
Contributor

@notmgsk @dwillmer This PR looks like it's going stale. Are you still interested in this change?

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.

3 participants