the appetizer (1/4). binary_operator #202
Conversation
…d the decoder shifter accordinly
…ing mathematica code. it was giving wrong operators before, now the operators are fine but signs are wonky
…to count qubit/fermion labels from 1 instead of 0
…ht) as a tool to append the same code intance several times
…sts for symbolicBinary. minor mods for python3 comp.
…ode, added an error if qubit index in a code is out of bounds
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
There was a problem hiding this comment.
This looks like a great contribution. I have a number of nit-picky comments that mostly pertain to the style guide and doc strings. This stuff is pretty obnoxious at first but they're standards that we try very hard to enforce and once you get used to them you don't even think about it. We have our reasons (we think anyways!). I will likely have another round of comments.
I would also suggest that, if you like, you edit the NOTICE and README to include your names and affiliations since these are valuable contributions to the library.
| # symbolic binary class for decoder definitions (arxiv 1712.07067) | ||
|
|
||
| import copy | ||
| import numpy as np |
There was a problem hiding this comment.
okay, here it is, perhaps the most despised rule of OpenFermion: we have a strict "no importing as" rule. I know it's kind of silly in this case since np is almost universally recognized as numpy. But at this point we should stay consistent with other parts of the code.
| @@ -0,0 +1,494 @@ | |||
| # symbolic binary class for decoder definitions (arxiv 1712.07067) | |||
There was a problem hiding this comment.
Please copy the Apache 2 license header that is in the rest of the OpenFermion files. This is a fine one-line description but it should go under the header and should be commented
"""Like this."""
rather than
# like this.
| components of decoders that are used to rotate terms in qubit basis to fermionic basis. | ||
|
|
||
| A SymbolicBinary represents a sum of terms. Each term can consist of two types of binary variables: | ||
| (n,'W') where n stands for the qubit index, or (1,'1') for constant 1. |
There was a problem hiding this comment.
Can you add a little more context to the description about when this class would be used and for what. Also, I think it is a good idea to put the arXiv paper reference in the doc strings for the class so that people looking through the online documentation can find the reference.
| A SymbolicBinary represents a sum of terms. Each term can consist of two types of binary variables: | ||
| (n,'W') where n stands for the qubit index, or (1,'1') for constant 1. | ||
|
|
||
| The SymbolicBinary class overloads operations for manipulation of binary functions obeying binary rules. |
There was a problem hiding this comment.
Please run the pep8 linter. This line is over 100 characters. We're old school - keep to 80 char per line please.
| self._check_terms() | ||
|
|
||
| def _check_terms(self): | ||
| """ |
There was a problem hiding this comment.
General comment: you don't need white space at the top and bottom of the doc strings. As an example just write:
"""Blah Blah Blah."""
|
|
||
| def _check_factor(self, term, factor): | ||
| """ | ||
| makes sure all factor in a term makes sense, cleans it up if there is multiplication with 1. |
| term: (list) of tuples of tuples. | ||
|
|
||
| """ | ||
| if not term: |
There was a problem hiding this comment.
Code is generally easier to read if try to avoid negatives in conditionals. For instance if you write "if term" and then what comes instead of "if not term"...
| """ | ||
| parse a string term 'w1 w2 w0' | ||
| Args: | ||
| term: (str) string representation of symbolicBinary term. |
There was a problem hiding this comment.
It is best if you write
term (str): string representation of SymbolicBinary term
I believe that is picked up in the optimal way by our sphinx documentation.
| @@ -0,0 +1,108 @@ | |||
| import unittest | |||
| from _binary_operator import SymbolicBinary | |||
There was a problem hiding this comment.
please update __init__.py in this PR so that you can just write from openfermion.ops import SymbolicBinary.
| from _binary_operator import SymbolicBinary | ||
|
|
||
|
|
||
| class SymbolicBinaryTest(unittest.TestCase): |
There was a problem hiding this comment.
These are nice tests but unfortunately you will need a few more. This is the downside to introducing a large class like this with all the magic methods. Coveralls will show you which lines are covered and which aren't. Apparently you are covering 156 relevant lines but missed 43.
|
It is now outdated but I also had a comment that you should update |
|
Oh wow thanks a lot for all the quick comments @babbush ! I just saw them, will work on them asap. |
|
You have my consent to use my contributions. |
|
@conta877 let me know when I should review again. Don't worry about the CLA bot at this point - it can be rather overzealous and I can override. I can see you both signed. |
|
@babbush will do! gonna be pushing/merging a bunch until then. sorry for the spam |
|
no more x's! @babbush I think (hope) we addressed all your comments. |
src/openfermion/ops/__init__.py
Outdated
| @@ -21,3 +21,4 @@ | |||
| from ._interaction_operator import InteractionOperator | |||
| from ._interaction_rdm import InteractionRDM | |||
| from ._quadratic_hamiltonian import QuadraticHamiltonian | |||
| from ._binary_operator import SymbolicBinary | |||
There was a problem hiding this comment.
we prefer to import things in alphabetical order, meaning this should be the first import. However, sometimes there is a problem with circular imports. If that is the case here, then it is fine.
|
oh sorry I just forgot about that! will do |
* fixed bohr/angstrom confusion in function description * translations from mathematica to python arxiv1712.07067 * work on those notimplementeds * WIP: decoder class adittion * modified decoder class. extractor implemented * no need for pauliaction function * introduction of symbolic binary class - no more decoder class * minor bugs fixed, applies binary rules to input now * started programming the BinaryCode class, work in progress * added count qubit function in binary operator, modified code.py * fixed morning brain bugs * implemented concatination and appending, not yet documented and debugged * added _shift function to symbolicBinary and integer addition. modified the decoder shifter accordinly * radd imul, NOT TESTED! - just copy paste should work? * fixed small bugs, added default codes, created transform * fixed the multiplication error and modified the transform code following mathematica code. it was giving wrong operators before, now the operators are fine but signs are wonky * fixed the transform and all the places where we accidentally started to count qubit/fermion labels from 1 instead of 0 * bug fixes: ordering matters to detect which terms should cancel * fixed code appending, introduced the integer multiplication (left+right) as a tool to append the same code intance several times * modified the multiply by 0 and 1 behavior in SymbolicBinary. added tests for symbolicBinary. minor mods for python3 comp. * fixed the condition for code concatenation, fixed a bug in checksum_code, added an error if qubit index in a code is out of bounds * added comments to binary_operator, more test, evaluate function * prep for first pull request: The appetizer, binary_operator only * cleaning up no more decoder.py * covering and python3 (i hope). those x's break my heart * started writing documentations and clipping lines, writing out numpy * prepping for merge merge * minor updates * description updates * init update * moved the binary_operator - no dependencies * added names to notice and readme
first pull request for #154
implementing arxiv 1712.07067
Decoders have binary variables w1, w2 that need to obey binary rules.
SymbolicBinary class takes care of that. they make up one component of a decoder.