Skip to content
This repository was archived by the owner on Oct 7, 2019. It is now read-only.

Clean up unitary_generator a bit#3

Merged
jotterbach merged 3 commits into
rigetti:masterfrom
thomdixon:generator-cleanup
Oct 10, 2017
Merged

Clean up unitary_generator a bit#3
jotterbach merged 3 commits into
rigetti:masterfrom
thomdixon:generator-cleanup

Conversation

@thomdixon
Copy link
Copy Markdown
Contributor

@thomdixon thomdixon commented Oct 7, 2017

  1. Use Python's abstract base classes to remove strict
    type assertions where they're not necessary
  2. Use Python's support for a more mathematical (and therefore
    verifiable) syntax for relations (e.g., x < y < z)
  3. Removes the use of divmod and np.log2 to calculate powers of 2
  4. Be more Pythonic in general

1) Use Python's abstract base classes to remove strict
   type assertions where they're not necessary
2) Use Python's support for a more mathematical (and therefore
   verifiable) syntax for relations (e.g., x < y < z)
3) Be more Pythonic in general.
@jotterbach jotterbach self-requested a review October 9, 2017 17:37
@jotterbach jotterbach self-assigned this Oct 9, 2017
Copy link
Copy Markdown
Contributor

@jotterbach jotterbach left a comment

Choose a reason for hiding this comment

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

Thanks @thomdixon for these valuable readability improvements!
It all looks good and I have only 2 comments to update the PR slightly.

Comment thread referenceqvm/unitary_generator.py Outdated
"permutation")
else:
# Don't permit NoneType or empty sequences, but allow 0
if args is None or (isinstance(args, Sequence) and not args):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the improvements here. args is a required argument and hence cannot be none, so we should be able to drop this check?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If you set args to None in master, you wind up with args being set to [None]. I wanted to prevent that situation to the best of my ability. I can remove it, if that situation is acceptable though.

raise TypeError("Need at least one qubit index to perform"
"permutation")
else:
# Don't permit NoneType or empty sequences, but allow 0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we need to adjust the docstring parameter based on this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Can do.

Copy link
Copy Markdown
Contributor

@jotterbach jotterbach left a comment

Choose a reason for hiding this comment

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

@thomdixon This looks good! Thanks a lot!

@jotterbach jotterbach merged commit c09805a into rigetti:master Oct 10, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants