quantumlib / OpenFermion

The electronic structure package for quantum computers.
Apache License 2.0
1.51k stars 374 forks source link

Inconsistency in basis transformations between code and paper #116

Closed kevinsung closed 6 years ago

kevinsung commented 6 years ago

I believe there is an inconsistency in how basis transformations of PolynomialTensor are treated between the code and the paper. I think it applies to the implementation of both one- and two-body basis changes, but I'll explain it for one-body. In Eq. (21) of the paper, the molecular orbital functions are transformed by the matrix U. This corresponds to a transformation of the creation operators by the same matrix U. In other words, the column vector of creation operators (a^\dagger_1, ..., a^\dagger_N)^T is multiplied on the left by U. Let's assume that U is real and call it R instead. Then equivalently, the row vector of creation operators (a^\dagger_1, ..., a^\dagger_N) is multiplied on the right by R^T. This implies that for a one-body tensor of type (1, 0), i.e., with terms of the form a^\dagger_p a_q, the one-body tensor matrix M should be transformed as M' = R M R^T. We put R on the left side which will cancel out the R^T which transforms the creation operators; similarly, the R^T on the right side cancels out the R which transforms the annihilation operators. However, in the current implementation of one_body_basis_change, the transformation is M' = R^T M R. Does this make sense to anyone? I'm happy to give a more detailed (prettier) explanation with LaTeX. If I'm correct, then I propose that we change the code to be consistent with the paper. Perhaps this can be resolved at the same time as #86 .

babbush commented 6 years ago

So you are suggesting that we are implementing R^T M R instead of R M R^T? @jarrodmcc do you have any concerns about this? If not, I am fine signing off on the change.

kevinsung commented 6 years ago

Yeah, the code for one_body_basis_change actually implements R^T M R, and I'm suggesting that it should be R M R^T instead. I think the code for two_body_basis_change would need to be changed too.

babbush commented 6 years ago

The thing is that rotation matrices have the property that R^T = R^{-1}, which is just a rotation in the other direction. So you could view what we're actually doing as (R^{-1}) M (R^{-1})^T. So its just rotating in the opposite direction. This doesn't seem like a significant problem to me. Can we not just change what is in the paper? I don't really have strong feelings though.

babbush commented 6 years ago

I should also mention that when changing the basis of an operator you should rotate in the opposite direction as when you change the basis of an RDM. I don't think we're being careful about that right now though.

kevinsung commented 6 years ago

Yes, this issue is simply a matter of convention, so in that sense it doesn't matter that much. I guess the choice is between whether we think of the rotation of the operator or the rotation of the RDM as the "inverted" one. I'd like to think of the rotation of the RDM as the inverted one, probably because the papers I've been reading lately deal mainly with operators and use this convention. Either way, it's a very easy change to make. I think the main reason to address this issue is to make the code and the paper match up, and since I like the convention of the paper, I want to keep the paper the way it is and change the code.

babbush commented 6 years ago

That sounds fine to me.

babbush commented 6 years ago

Closed by #117