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

Add QuadraticHamiltonian class #482

Merged
merged 27 commits into from Apr 8, 2022
Merged

Conversation

kevinsung
Copy link
Contributor

@kevinsung kevinsung commented Dec 22, 2021

Summary

Part of #460. Adds a data structure to represent quadratic Hamiltonians.

Details and comments

I have implemented a method _fermionic_op to convert a QuadraticHamiltonian to a FermionicOp. For now, I have kept that method hidden because I'm not sure what general approach we should take to converting between operator types, e.g., whether we want to have instance methods or whether we want to have a functional style like qiskit_nature.fermionic_op(quadratic_hamiltonian)

@coveralls
Copy link

coveralls commented Dec 23, 2021

Pull Request Test Coverage Report for Build 2109658637

  • 124 of 124 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 83.532%

Totals Coverage Status
Change from base Build 2103855677: 0.2%
Covered Lines: 9029
Relevant Lines: 10809

💛 - Coveralls

@kevinsung
Copy link
Contributor Author

The CI failure is unrelated to this PR.

      File "/home/runner/work/qiskit-nature/qiskit-nature/test/algorithms/pes_samplers/test_bopes_sampler.py", line 154, in test_vqe_bootstrap
    np.testing.assert_almost_equal(result.energies, ref_energies)

@woodsp-ibm
Copy link
Member

The CI failure is unrelated to this PR.

Was just fixed so I updated your branch - should pass now

@kevinsung kevinsung force-pushed the quad-ham branch 2 times, most recently from 2b8ad87 to 34fda9d Compare February 9, 2022 13:53
Copy link
Member

@mrossinek mrossinek left a comment

Choose a reason for hiding this comment

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

As outlined here we can proceed with this PR.

@kevinsung
Copy link
Contributor Author

@mrossinek I have addressed all your comments besides the one related to the copyright checker. If needed, I can manually squash my commits into a single one with a 2022 timestamp.

@mrossinek
Copy link
Member

I have addressed all your comments besides the one related to the copyright checker. If needed, I can manually squash my commits into a single one with a 2022 timestamp.

Sounds like the copyright should be fine as only the last year is being checked. Please fix CI and then we can progress with merging this 👍

@woodsp-ibm
Copy link
Member

This needs a reno release note for this new feature

@kevinsung
Copy link
Contributor Author

@woodsp-ibm I've added a release note.

Copy link
Member

@mrossinek mrossinek left a comment

Choose a reason for hiding this comment

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

Just a minor comment, other than that this lgtm 👍

"""The number of modes this operator acts on."""
return self._num_modes

def to_fermionic_op(self) -> FermionicOp:
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering if we should rename this to to_second_q_op which is inline with the namings in the properties module.
If we (in the future) design a common base class for some matrix container that will likely be the name of this function, avoiding the deprecation of this one.

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 think the current name is much better because it precisely describes what the method does. to_second_q_op is confusing because QuadraticHamiltonian is already a "second-quantized op" (though not a SecondQuantizedOp). From the perspective of a user trying to discover this method, I think they are more likely to guess the current name.

@kevinsung
Copy link
Contributor Author

@woodsp-ibm I have addressed your comments. Please take another look.

@woodsp-ibm
Copy link
Member

I think you missed the f from the string they now need to be like f"my value {x}" for the vars to be formatted in.

Also although I put the comment in one place for the error message I was meaning it could apply to a number of the error messages in the various checks to show the failing values where that's applicable.

@kevinsung
Copy link
Contributor Author

@woodsp-ibm Done.

@mrossinek mrossinek merged commit b0cddf8 into qiskit-community:main Apr 8, 2022
@kevinsung kevinsung deleted the quad-ham branch April 8, 2022 12:32
Anthony-Gandon pushed a commit to Anthony-Gandon/qiskit-nature that referenced this pull request Jun 3, 2022
* add QuadraticHamiltonian class

* validate input

* expose _fermionic_op as to_fermionic_op

* use tuple labels and merge loops

* update docs

* add num_modes argument

* document new errors

* add num_modes property

* update doc

* spelling

* mypy

* from __future__ import annotations

* add release note

* trigger CI

* fix spelling

* use type declaration instead of comment

* fix doc and error message

* fix and improve error messages

Co-authored-by: Panagiotis Barkoutsos <bpa@zurich.ibm.com>
Co-authored-by: Manoel Marques <manoel.marques@ibm.com>
Co-authored-by: Max Rossmannek <oss@zurich.ibm.com>
Anthony-Gandon pushed a commit to Anthony-Gandon/qiskit-nature that referenced this pull request Aug 11, 2022
* add QuadraticHamiltonian class

* validate input

* expose _fermionic_op as to_fermionic_op

* use tuple labels and merge loops

* update docs

* add num_modes argument

* document new errors

* add num_modes property

* update doc

* spelling

* mypy

* from __future__ import annotations

* add release note

* trigger CI

* fix spelling

* use type declaration instead of comment

* fix doc and error message

* fix and improve error messages

Co-authored-by: Panagiotis Barkoutsos <bpa@zurich.ibm.com>
Co-authored-by: Manoel Marques <manoel.marques@ibm.com>
Co-authored-by: Max Rossmannek <oss@zurich.ibm.com>
Anthony-Gandon pushed a commit to Anthony-Gandon/qiskit-nature that referenced this pull request May 25, 2023
* add QuadraticHamiltonian class

* validate input

* expose _fermionic_op as to_fermionic_op

* use tuple labels and merge loops

* update docs

* add num_modes argument

* document new errors

* add num_modes property

* update doc

* spelling

* mypy

* from __future__ import annotations

* add release note

* trigger CI

* fix spelling

* use type declaration instead of comment

* fix doc and error message

* fix and improve error messages

Co-authored-by: Panagiotis Barkoutsos <bpa@zurich.ibm.com>
Co-authored-by: Manoel Marques <manoel.marques@ibm.com>
Co-authored-by: Max Rossmannek <oss@zurich.ibm.com>
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

6 participants