Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

N-body rotations #117

Merged
merged 12 commits into from
Dec 3, 2017
Merged

N-body rotations #117

merged 12 commits into from
Dec 3, 2017

Conversation

fangzh-umich
Copy link
Contributor

Resolves issues #86 and #116.

I implemented general n-body rotations of PolynomialTensor using np.einsum, with the option optimize = True to ensure that we trade space for time (e.g. in the 2-body term, use the O(N^5) algorithm instead of the O(N^8) one).

Also, as per issue #116, @kevinsung and I changed the convention for the rotation matrix so that it's consistent with the release paper.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@fangzh-umich
Copy link
Contributor Author

fangzh-umich commented Dec 1, 2017 via email

@googlebot
Copy link

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 cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

"""Change the basis of an general interaction tensor.

M' = R^T.M.R where R is the rotation matrix, M is the general tensor
and M' is the transformed general tensor.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should it be R.M.R^T? Also, perhaps there is a better way to express what this does, as transpose seems to me to be a 2-d concept; I might be wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a rotation "matrix", of course it's 2-d.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Haha ok.

# "optimize = True" does greedy optimization, which will be enough here
transformed_general_tensor = numpy.einsum(subscripts,
general_tensor,
*rotation_matrices,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does the * in *rotation_matrices do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a Python syntax to pass a variable number of parameters to a function. For example, if key is (0,0,0) then this is equivalent to numpy.einsum(subscripts, general_tensor, rotation_matrix, rotation_matrix, rotation_matrix).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool!

@kevinsung
Copy link
Collaborator

kevinsung commented Dec 1, 2017

For testing, perhaps we can compute the rotated matrix in two different ways, one using your new function, and another using a naive sum, like Eq. (22) in the paper, and make sure they match up? It doesn't seem like a great test but I can't think of a better way right now.

Perhaps the functions one_body_basis_change and two_body_basis_change should be removed completely?

Yes, of course I'm okay with my commits being used in this PR.

@fangzh-umich
Copy link
Contributor Author

fangzh-umich commented Dec 1, 2017

I'm not sure how to increase the coverage of my test.

First, there is a maximum order restriction because of a limitation of numpy.einsum: it uses letters to indicate subscripts, and there are only 52 (upper case and lower case) in the English language. I don't expect anyone to actually hit it, and I hope a future version fixes this (probably when numpy itself overcomes this limitation).

Second, my code has no use for one_body_basis_change and two_body_basis_change, but I left them in because __init__.py explicitly imports those functions. I don't know what those imports are for, so I didn't just remove them.

@kevinsung
Copy link
Collaborator

Ok then, this looks good to me after you address the "R.M.R^T" thing.

@fangzh-umich
Copy link
Contributor Author

fangzh-umich commented Dec 1, 2017

Actually, my test just showed that numpy complains about "too many subscripts in einsum" before it gets to 26 52 indices.

@fangzh-umich
Copy link
Contributor Author

Also, I realized that you defined PolynomialTensorError, but you never raised it, and you never exported it (meaning that it's not visible to the user). Since my code raised that exception, I exported it.

But am I supposed to raise some other exception like ValueError? What is the intended use of PolynomialTensorError?

@fangzh-umich
Copy link
Contributor Author

I'm still not sure what I should do with one_body_basis_change and two_body_basis_change. Should I unexport them or should I test them? Should I export my general_basis_change too?

@kevinsung
Copy link
Collaborator

Don't export PolynomialTensorError, and raise a ValueError instead.

I think we should remove one_body_basis_change and two_body_basis_change, and also not export general_basis_change. I don't see any reason general_basis_change needs to be exposed to users, but somebody else (maybe @babbush ) should have the final say on that.

I don't know what to say about einsum complaining before there are 26 indices.

Copy link
Contributor

@babbush babbush left a comment

Choose a reason for hiding this comment

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

This mostly looks good, thanks for the contribution. Feel free to add yourself to the alphabetical part of the credits in the NOTICE and the README.

rotation_matrix)
return transformed_one_body_tensor
order = len(key)
if order > 26:
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this order 26 limit coming from? I suggest not having a maximum order restriction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It comes from using the numpy.einsum function, which uses letters for indices, and there are 26 upper case and 26 lower case in total (and I need two letters for each dimension of the tensor). However, actually numpy complains about "too many subscripts in einsum" before the 26 limit is reached.

I don't think anything more than 16 is useful in any case.

from ._polynomial_tensor import (PolynomialTensor,
one_body_basis_change,
two_body_basis_change)
from ._polynomial_tensor import PolynomialTensor
Copy link
Contributor

Choose a reason for hiding this comment

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

we like to have alphabetically organized imports when possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but I didn't introduce this problem. If the person originally writing this code didn't have any particular reason to do it this way, I will change that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turns out that I can't change the order freely, because some files imported later depends on some earlier imports having been processed.


def two_body_basis_change(two_body_tensor, rotation_matrix):
"""Change the basis of 2-body interaction tensor such as 2-RDM.
# 'aA,bB,cC,dD'
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a line or two explaining what is happening here a bit more? I am a little confused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm doing the basis change through a single call of numpy.einsum, which requires constructing a fitting subscript string. For example, for the (1, 1, 0, 0) tensor, I'm doing numpy.einsum('abcd,Aa,Bb,Cc,Dd', general_tensor, rotation_matrix.conj(), rotation_matrix.conj(), rotation_matrix, rotation_matrix).

Although I just realized my comment is incorrect after the convention change. I will fix that, and at the same time try to make it more explicit.

Copy link
Contributor

@babbush babbush left a comment

Choose a reason for hiding this comment

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

👍

@babbush babbush merged commit 98355c6 into quantumlib:master Dec 3, 2017
@fangzh-umich fangzh-umich deleted the n_body_rotations branch December 4, 2017 02:35
philipp-q pushed a commit to philipp-q/OpenFermion that referenced this pull request Sep 2, 2020
* added test for diagonalizing quadratic Hamiltonians

* Generalized the basis change functions

* Changed the test to test with complex rotation matrices

* changed transpose convention for one-body rotation

* Improved the comment of general_basis_change

Now the comment better expresses the general algorithm.

* Added test for high-order tensor rotation

* Raise ValueError instead of PolynomialTensorError

* Removed [one|two]_body_basis_change altogether

* Improved the comment for the main procedure

* Added name into NOTICE and README
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants