transform function 3/4 #208
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 |
| SymbolicBinary) | ||
|
|
||
|
|
||
| def _extract(term): |
There was a problem hiding this comment.
Is this helper function used in more than one place? If not, I don't think it should be its own function - wouldn't it make sense just to put this code in the "extractor" function? Again, I am very weary of these private functions.
| Returns (QubitOperator): superposition of Pauli-strings | ||
|
|
||
| Raises: | ||
| ValueError: if the action is not 'W' |
There was a problem hiding this comment.
Why "W" where does this convention come from exactly? I am concerned that this design is rather opaque.
There was a problem hiding this comment.
I'm not sure that is a very satisfying answer for you, but 'w' is just labeling the binary variables. It stems from the SymbolicBinary class. At some point writing the paper I had to decide for symbols for the binary vectors of the Fermion modes and qubit configuration. I just called them \nu and \omega. We don't need \nu here, but \omega survived as 'w'.
There was a problem hiding this comment.
So I am starting to question whether the data structure for SymbolicBinary is implemented in a reasonable fashion. What other value can the action take? Just '1'?. In this case, it seems to me that the whole data structure is overcomplicated. Instead of storing (1, 'W'), (2, 'W') shouldn't you just store (1,2)? To represent constants you can use "none", or something like that.
There was a problem hiding this comment.
I actually experimented with different ways of defining the SymbolicBinary class - even tried the (1,2,..) version which would indeed simplify things a lot for the operations - couple of problems I had with that was the constant definition and user input.
for constant indeed a string is a good idea, did not think of that. As for user definitions would we not allow for str inputs? would we stick to 'W' in str inputs - if not what if the user gives 1+q1+w2+z3 is that ok? or do we assume that anyone thats using the symbolicBinary class knows what they are doing?
There was a problem hiding this comment.
It's definitely more streamlined this way, you are right. We basically made it like it is now just because appears not to be that different from the data structures of FermionOperator and QubitOperator. But it's your call: would you like us to change (1, 'W'), (2, 'W') into (1,2) and so on?
There was a problem hiding this comment.
Yes, I think this is a good change to make. I would actually suggest "None", for a constant, but you could put a string. It's very easy to write "if action is None:" and it is probably less arbitrary than any string you could choose. I can tell you were thinking of inheriting from SybolicOperator, but since you didn't, I think the change makes sense.
Regarding string initialization, I think that is still fine. You can probably allow them to use any char in fact. So if they wrote "a1 a2 + a3 a4" or "W1 W3 + W3 W4" it would do the same thing. Another option is to represent the term a1 a2 with (1 2) in the string input also. So you could write (1 2) + (3 4). I personally find the second option cleaner but that is up to you.
Another thing to think about is whether you want to enforce numerical order. For instance, if you want to disallow people from having a term (3,1) as opposed to (1,3). If you do this, you can certainly perform certain numerical operations faster since you know the list is sorted. But unless numerical performance is a bottleneck it is not necessary to specialize the magic methods for this now (but it might still he a good idea to put systems in place that prevent having both (1 3) and (3 1).
Sorry I did not catch this before!
There was a problem hiding this comment.
Ok, we'll make the change. We already take care of several mishaps during the input, like a messed up order or additions with constants other than one, and so on.
There was a problem hiding this comment.
I'm ok with it all but None. its a dangerous name to use for a variable. certain data structures are built to ignore None's by default - and defining 1 as None is confusing to me.
There was a problem hiding this comment.
That's fine. If you feel a string would be better that seems reasonable.
|
Note that I am hoping that PR 4/4 can also come with a small example ipython notebook, or perhaps a new section in our main demo, demonstrating how this code works in a pedagogical fashion. That would be really great and I am sure such examples would increase adoption. |
|
Let me know when you are ready for review, thanks for making these changes! |
|
we are ready for review! (I'll improve the coverage tonight) |
| Returns (QubitOperator): superposition of Pauli-strings | ||
|
|
||
| Raises: | ||
| ValueError: if the _action is not 'W' |
There was a problem hiding this comment.
I update docs. I think this was left over from prior implementation.
There was a problem hiding this comment.
hmm I remember fixing that - meaning I may have messed something up with my merging. I should check to see whats happening before you approve.
There was a problem hiding this comment.
oh nvm - I totally missed that one and fixed the same error within the raise.
|
Actually, test coverage looks okay currently! Definitely add more tests if you like though. |
|
for the notebook, do you prefer a section in main demo or separate? |
|
Either would be great! Maybe easiest to add to main demo. |
|
Should I go ahead and merge? |
|
yup! there will be a 4/4 with transform functions and addition to the main demo |
* 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 * started writing documentations and clipping lines, writing out numpy * merging with the merger * updating binary operator based on comments * updating the binary_ops * updates based on comments * init update * moved binaryop to first import * added names into notice and readme * started documentation in _code_operator.py, added parity code / (K=1) and (K=2) segment code / K=1 binary addressing code * encoders are sparse matrices now * added tests for code_operator * doc strings for the transform, fixed bug in the initialization of SymbolicBinary, made changes suggested by review, carried some of them over to files to be pulled later * updates based on pull request comments * merging changes with merger * fixing possible integer checking errors * cleaned version of transform function, needs testing and timing - seems to work so far * fixed BK transforms. indexing errors * added tests, pep8 and Ryan compliance mods * added some style to the functions file * renaming, re-structuring * renaming again * minor changes * pep8 * prep for pull request * import order * changed the symbolicBInary class * docstring cleanup * bug docstring fixes * bug fix in the evaluate method, pep8 the _binary_operator file * merge with upstream and additional tests to SymbolicBinary class * merge prep * new symbolicBinary datastructure, new tests * more tests
3rd pull request for #154