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

Use eigh, eigsh in NumPyEigensolver #6987

Merged
merged 15 commits into from
Oct 13, 2021
Merged

Conversation

ikkoham
Copy link
Contributor

@ikkoham ikkoham commented Sep 7, 2021

Summary

If NumPyEigensSolver knows that the given operator is hermitian, we should use eigh or eigsh.
The eigenvalue becomes float from complex.

Details and comments

image

@ikkoham ikkoham requested review from manoelmarques, woodsp-ibm and a team as code owners September 7, 2021 07:28
@woodsp-ibm
Copy link
Member

What are the 2 notebooks here in this PR - were they added by mistake?

@ikkoham
Copy link
Contributor Author

ikkoham commented Sep 15, 2021

Oh... my mistake... Thanks.

test/python/opflow/test_op_construction.py Outdated Show resolved Hide resolved
test/python/algorithms/test_numpy_eigen_solver.py Outdated Show resolved Hide resolved
test/python/algorithms/test_numpy_eigen_solver.py Outdated Show resolved Hide resolved
test/python/algorithms/test_numpy_eigen_solver.py Outdated Show resolved Hide resolved
test/python/opflow/test_pauli_sum_op.py Outdated Show resolved Hide resolved
ikkoham and others added 2 commits October 8, 2021 10:36
Co-authored-by: Jake Lishman <jake@binhbar.com>
@ikkoham
Copy link
Contributor Author

ikkoham commented Oct 8, 2021

@jakelishman Thank you for your review. I made some mistakes.

Comment on lines +173 to +179
def is_hermitian(self) -> bool:
"""Return True if the operator is hermitian.

Returns: Boolean value
"""
return (self.to_spmatrix() != self.to_spmatrix().getH()).nnz == 0

Copy link
Member

Choose a reason for hiding this comment

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

With floating-point maths, there's always the thorny question of tolerances to consider. I don't really know if it's appropriate for your use-cases, here, though.

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 agree. Personally, I think OperatorBase should inherit TolerancesMixin. However, the OpFlow implementation does not do so. e.g. https://github.com/Qiskit/qiskit-terra/blob/d2013d1812c9229455c1cfb1c79c560fecbc5dfe/qiskit/opflow/primitive_ops/pauli_op.py#L90

The concept of equality is different between Opflow and quantum_info.operators. opflow is exact and qinfo allows errors.
I believe that it should be unified in the future, but this is beyond the scope of this PR, so please leave it for now.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds fine to me, then - definitely best just to follow what's already there in this case!

@jakelishman jakelishman added automerge Changelog: New Feature Include in the "Added" section of the changelog mod: opflow Related to the Opflow module labels Oct 13, 2021
@mergify mergify bot merged commit 12f2b52 into Qiskit:main Oct 13, 2021
@ikkoham ikkoham deleted the numpysolver-eigh branch October 14, 2021 09:09
@kdk kdk added this to the 0.19 milestone Nov 15, 2021
ElePT pushed a commit to ElePT/qiskit that referenced this pull request Jun 27, 2023
* Use eigh, eigsh

* add is_hermitian method

* add is_hermitian method to OperatorBase

* rm unnecessary files

* modify releasenot

* Apply suggestions from code review

Co-authored-by: Jake Lishman <jake@binhbar.com>

* fix test name

* Update releasenotes/notes/add-opflow-is-hermitian-method-6a461549e3c6b32c.yaml

Co-authored-by: Jake Lishman <jake@binhbar.com>

Co-authored-by: Jake Lishman <jake@binhbar.com>
ElePT pushed a commit to ElePT/qiskit-algorithms-test that referenced this pull request Jul 17, 2023
* Use eigh, eigsh

* add is_hermitian method

* add is_hermitian method to OperatorBase

* rm unnecessary files

* modify releasenot

* Apply suggestions from code review

Co-authored-by: Jake Lishman <jake@binhbar.com>

* fix test name

* Update releasenotes/notes/add-opflow-is-hermitian-method-6a461549e3c6b32c.yaml

Co-authored-by: Jake Lishman <jake@binhbar.com>

Co-authored-by: Jake Lishman <jake@binhbar.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog mod: opflow Related to the Opflow module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants