Givens rotations #31
Givens rotations #31
Conversation
There was a problem hiding this comment.
This pull request looks really great. Very good job with testing too! I have only one annoying suggestion. Generally speaking (there are some exceptions), we really try to give all variables descriptive names instead of things like "G", "V". While I do happen to know why you choose these names in context, such names make it much harder for those less familiar to figure out what is going on. We really don't care about this sort of thing in tests but if you could change a few of the more generically named variables to things more descriptive, it would be greatly appreciated.
|
|
||
| import numpy | ||
|
|
||
| from openfermion.config import * |
There was a problem hiding this comment.
usually it's a bad idea to import * because it pollutes the namespace in ways that aren't clear unless you actually look in the file. Instead, just import explicitly EQ_TOLERANCE, which is I think all you need.
| @@ -0,0 +1,293 @@ | |||
| { | |||
There was a problem hiding this comment.
Making an ipython notebook for this is really going above and beyond. Great work! If you do want to include this though I will ask that you add a very primitive test that checks to make sure that the ipython notebook runs without throwing any errors (no need to test further). The reason we do this is so that examples in ipython notebooks don't get silently broken by later updates (which used to happen to use a lot). I implemented such a test for our demo in tests/_example_test.py
You can add the test for your ipython notebook to that file as well. Just copy the same format. Note that some aspects of this way of testing are a little hacky. The worst part is that you will need to add those lines with "chdir" at the top of the ipython notebook (see what I did in the demo). If you have a better idea for how to get these tests to pass on travis though, do let us know!
|
Ok I changed some of the variable names in the function (I didn't change the tests but let me know if you want that too). However, I left the single capital letter names for matrices in the function description; let me know if you think that should be changed too. |
|
overriding CLA who has again lost its mind. |
Zhang and I worked on this together, so I want to make sure he gets credit. This is helpful for compiling circuits to prepare Slater determinants. Since it's a little involved, I made a Jupyter notebook to explain what the code is doing. In short, it takes a matrix of orthonormal rows and decomposes it into a unitary times a diagonal matrix times a sequence of Givens rotations, keeping track of which rotations can be done in parallel. The unitary is introduced because it saves some work in preparing the Slater determinant.
The test cases generate a random starting unitary each time using Scipy's QR decomposition algorithm. I'm not sure if that's appropriate; I can also hard-code some matrices in.
I understand that this is kind of a stand-alone contribution right now, but I think it could fit in. For instance, it can be used in conjunction with a routine that diagonalizes InteractionOperators that have only single-body interactions. Zhang and I are also currently working on code to prepare ground states of general quadratic Hamiltonians (not necessarily particle-number-conserving). I think functionalities for dealing with these types of Hamiltonians would be useful.