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

Get ground states by particle number #33

Merged
merged 15 commits into from
Oct 14, 2017
Merged

Get ground states by particle number #33

merged 15 commits into from
Oct 14, 2017

Conversation

kevinsung
Copy link
Collaborator

This lets you compute the ground energy and ground states of a particle-conserving Jordan-Wigner encoded sparse Hermitian operator at a particular particular number.

@babbush babbush self-requested a review October 14, 2017 04:02
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 is a helpful function that we have been meaning to add. I have a few questions but mostly looks good.

particle number.

Args:
sparse_operator: A Jordan-Wigner encoded sparse operator.
Copy link
Contributor

Choose a reason for hiding this comment

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

While we are not always consistent with this, we try to indicate the type of all args / returns by specifying it in parenthesis right after the name. For instance, you would write:
particle_number(int): The particle number at which to compute ground states

This is perhaps even more important for something like "ground_states". You should specify both that the output is a list but also what are the elements of the list (in this case, a numpy array).


# Get the ground energy and initialize list to store the corresponding
# ground states
ground_energy = sorted(eigvals)[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps I am missing something but it seems to me that you are sorting the eigvalues but not the vectors. It seems strange that the eigenvalues returned would be in a potentially different order than the vectors to which they belong. Should you perhaps do numpy.argsort and use that mask to resort both the eigenvectors and values?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sorted actually returns a new list, so this doesn't sort the eigenvalues. However, I just found out that numpy returns the eigenvalues in sorted order anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh my bad, I think I missed the zero at the end here and thought you were returning a list of eigenvalues.

particle_number,
n_qubits)
dense_restricted_operator = restricted_operator.toarray()
eigvals, eigvecs = numpy.linalg.eigh(dense_restricted_operator)
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be missing something here but it looks like you are doing a dense diagonalization and then returning all of the eigenvectors and values associated with the nonzero eigenvalues. In this case, the function is not really about ground states, it is about all the eigenstates, right?

If you did want to specialize it to the low energy part of the spectrum I would think you would want to keep things sparse and use scipy.sparse.linalg which uses the lanczos routine (much faster when just trying to get the extremal eigenvalues of a sparse matrix).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking about the case that the ground space within a particle number sector was degenerate and spanned the whole sector, then we wouldn't get all the eigenvectors. I hadn't thought much about a better way to get around that or if it's an issue.

Copy link
Contributor

@babbush babbush Oct 14, 2017

Choose a reason for hiding this comment

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

Okay so it sounds like your intention is a function that just gets the low energy nontrivial eigenvalues (so you are after the ground state) but you are worried about the case when there is only one nonzero eigenvalue and many associated eigenvectors? I do not think that is likely to happen.

But I realize that there is a different problem with using lanczos in this situation. The problem is that if the lowest nontrivial eigenvalue is greater than zero you are in trouble. Because then the lowest ones will all be zeros.

I would perhaps suggest keeping your code as it is but reselling it as code to get the nontrivial part of the entire spectrum (so all eigenstates and eigenvalues that are not zero eigenvalue). Perhaps call the function "number_restricted_diagonalization" or something like that. Would that be okay?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, I don't understand the problem you pointed out with lanczos. It seems to me that if we used it instead of the dense routine, we would still get the same eigenvalue that I denoted ground_energy. The ground energy generally refers to the lowest eigenvalue, even if it's 0, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah wait, I should have read your first comment more clearly. This function returns the lowest eigenvalue, not the nonzero eigenvalues.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At least that's what it's supposed to do!

Copy link
Contributor

@babbush babbush Oct 14, 2017

Choose a reason for hiding this comment

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

Oh right, sorry (a little tired over here). I was thinking for some reason that the jw_number_restrict code just projects out parts of the operator in the wrong particle sector. Were that the case, you'd be left with an operator that has a bunch of trivial zero eigenvalues. So that is what I was complaining about. But I realize now that it actually gives you the smaller operator.

So indeed, you should be fine using lanczos. Does that work for you? I would not be too worried about a bunch of degenerate eigenvalues. Perhaps you can request that lanczos return the lowest three or four eigenvalues and raise a warning if by some chance they are all the same (the warning would be that there might be yet more degenerate eigenvalues). Perhaps you can have an optional parameter which switches between the dense and sparse diagonalization so one can switch to the dense routine if they get the warning when using the sparse routine. Or perhaps the code just does this automatically.

How does that sound?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that's a great idea!

@kevinsung
Copy link
Collaborator Author

Because I want the flexibility, I included a keyword argument to specify the number of eigenvalues to request from the sparse eigensolver. Another thing is that it automatically switches to using the dense eigensolver if the restricted operator is too small; I considered adding a warning for that but decided it's not important.

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.

LGTM

@babbush babbush merged commit dc1d8d1 into quantumlib:master Oct 14, 2017
@kevinsung kevinsung deleted the get_ground_states_by_particle_number branch October 14, 2017 17:49
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.

None yet

2 participants