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

Introduction of Property objects #220

Merged
merged 81 commits into from Jun 25, 2021

Conversation

mrossinek
Copy link
Member

@mrossinek mrossinek commented Jun 1, 2021

Summary

This is a very early WIP for #167.

Details and comments

As proposed in the aforementioned issue as well as the Epic #148, we want to introduce some modular Property objects, in order to containerize the QMolecule and make it fit better into the rest of the updated stack. This PR will make the first step into this direction. Below is a list of the changes:

  • Add Property objects for:
    • ElectronicEnergy
    • ParticleNumber
    • AngularMomentum
    • Magnetization
    • DipoleMoment
    • whatever comes out of the WatsonHamiltonian
  • generalize the electronic integrals to n-body terms
  • provide the means for arbitrary (almost) orbital basis transformation (at least the addition is of new bases is straight forward via the Basis(Enum))
  • leverage @jlapeyre's 2-body integral symmetry utilities as part of _2BodyElectronicIntegrals (EDIT: postponed)
  • tie the properties into the StructureProblems:
    • electronic (partially done, not very nice atm)
    • vibrational
  • add unittests
  • add documentation

The tying into the Problem classes will be improved in the future. The details of that implementation depend on steps outlined in #148. The same goes for the interpret() method stubs. I have them there right now because I think they could fit well, but they will only become useful if/once we reach the last implementation step.

⚠️ The API of this PR is by no means final. Public/private methods are bound to change. So are the names of all classes and methods.

@mrossinek
Copy link
Member Author

I started working on the WatsonHamiltonian and Bosonic/HarmonicBasis equivalents for this framework. As its not really fitting together yet I have not pushed these yet but I will update it here tomorrow. In the meantime hopefully the CI passes now.

@mrossinek
Copy link
Member Author

mrossinek commented Jun 3, 2021

More WIP commits. Things I am not yet satisfied with/sure about which need to be addressed as part of this PR:

  • the Basis handling (still not perfect but good enough for this first part of a series of PRs)
  • the fact that DipoleMoment contains all three axes without subcomponents

Things that will likely be addressed in the future:

  • some properties are mostly copied from the previously existing integral calculators. However, in some cases the label could be constructed directly in a simpler fashion
  • the interpret method stubs will only be useful if we proceed with steps 2 and 3 of QMolecule Design  #148
  • if we do decide to go there, we probably need an overall container of these properties. This is what ElectronicDriverResult and VibrationalDriverResult may become

This was referenced Jun 21, 2021
@mrossinek mrossinek mentioned this pull request Jun 23, 2021
woodsp-ibm
woodsp-ibm previously approved these changes Jun 23, 2021
@mrossinek mrossinek dismissed stale reviews from dlasecki and pbark June 24, 2021 05:29

All requested changes are implemented.

if self._basis == ElectronicBasis.SO:
return self._matrices # type: ignore

matrix_a = self._matrices[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we introduce a special type to avoid accessing by an index?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really. This tuple of matrices gets stored in the base class ElectronicIntegrals. It's length depends on which subclass we are dealing with (length is 2 in OneBodyElectronicIntegrals but 4 in TwoBodyElectronicIntegrals). Introducing a special type (e.g. a NamedTuple) would require registering this type in the base class which would need to be updated when we decided to add e.g. ThreeBodyElectronicIntegrals.

Given that the length and type of the matrices is asserted at construction time (I extracted the validation methods as you suggested above) I think this is okay as is.

pbark
pbark previously approved these changes Jun 24, 2021
@mrossinek mrossinek dismissed stale reviews from pbark and woodsp-ibm via 2609b7d June 25, 2021 08:13
@mrossinek mrossinek requested a review from dlasecki June 25, 2021 08:14
@mrossinek mrossinek merged commit b4a46bf into qiskit-community:main Jun 25, 2021
@mrossinek mrossinek deleted the containerized_qmolecule branch June 25, 2021 08:48
@mrossinek mrossinek mentioned this pull request Jul 5, 2021
7 tasks
Anthony-Gandon pushed a commit to Anthony-Gandon/qiskit-nature that referenced this pull request May 25, 2023
* [WIP] ElectronicEnergy Property object

* Generalize to n-body integrals

* Extract fermionic op builder for electronic energy

* [WIP] ParticleNumber Property

* Fix lint

* Add remaining aux_op-based properties

* Make tests slow again (as they used to be)

* Fix some matrix dimensions

We really need to properly handle different bases...

* WIP: prepare proper Basis handling

* Add simple BasisTransform

* Remove register_length from Property base class

* Do not use future annotations to support Python 3.6

* Fix lint

* Fix _2BodyElectronicIntegrals.to_spin

* Fix mypy in CI

* Very, VERY (!!!) early PoC vibrational integrals

* WIP: some improvements

* Add OccupiedModals property

* Some cleanup

* Fix spell

* Fix lint

* Allow empty operator initialization

* Revert "Allow empty operator initialization"

This reverts commit 3692340.

* Fix empty op case in VibrationalOp

* Simplify some properties

* Some naming updates

* Fix DipoleMoment

* Restructure properties submodule

* Fix style

* Fix mypy

* Fix spell

* Prepare property unittests

* Prepare properties documentation

* Address TODOs in properties.electronic

* Fix spell

* Address TODOs in properties.vibrational

* Add TODOs in structure problem classes

* Add unittests for ElectronicIntegrals

* Add unittests for HarmonicBasis

* Remove unused test directories

- electronic bases are merely static files which cannot really be tested
- vibrational integrals can basically only be tested as part of the vibrational basis tests

* Add HarmonicBasis operator construction tests

* Fix lint

* Remove unused builder utilities

Unfortunately, the UVCC and CHC unittests still rely on one of the removed, private methods.
For now, I moved them to the unittest suite but we will properly migrate those tests in the future.

* Fix auto-generated docs stubs

* Add some TODO marks for the future

* Fix spell

* De-duplicate code

* Use names for special ints/floats

* Extract input type validation

* Make naming consistent

* Remove unused imports

* Extract raw testing resources

* Fix docstring titles

* Fix Property valid type assertion

* Add Property base class tests

* Add vibrational property tests

* Add electronic property tests

* Fix linters

* Update docstring

* Remove interpret method stub from subclasses for now

* Make ElectronicBasisTransform variables public

* Extract common base class for ElectronicEnergy and DipoleMoment

* Replace raw assert statements

* Expose ERI_TRUNCATION_LEVEL overwrite option

* Fix linters

* Fix mypy

* Remove unused import

* Relocate qiskit_nature.properties to qiskit_nature.properties.second_quantization

* Fix mypy in CI

* Add missing docs

* Extract validation methods

* Extract integral threshold

* Simplify OccupiedModals

Co-authored-by: Dariusz Lasecki <dal@zurich.ibm.com>

* Replace bare assert statements

Co-authored-by: Dariusz Lasecki <dal@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