Skip to content
This repository has been archived by the owner on Dec 7, 2021. It is now read-only.

Ground state interface #1288

Merged
merged 201 commits into from
Oct 14, 2020

Conversation

pbark
Copy link
Collaborator

@pbark pbark commented Sep 29, 2020

Summary

Introducing Ground State calculation interface. This PR will introduce the qubit_transformations as the object responsible for taking the problem instance (driver) and producing the qubit operator and auxiliary operators. Also the base Ground state calculation along with MES ground state calculation and AdaptVQE ground state calculation are here.

Details and comments

Panagiotis Barkoutsos and others added 30 commits May 28, 2020 19:56
This properly implements the AdaptVQEGroundStateCalculation class.
The idea of this class is to allow easy use of the VQEAdapt algorithm
which was previously extended to support the MinimumEigensolver
interface. This Calculation class is solely for the use with this
algorithm which is why it does not support setting a different solver.
Instead, it requires setting a QuantumInstance object through its
initializer.

The changes which this requires include the following:

- fixing the GroundStateCalculation class
- fixing the MinEigensolverGroundStateCalculation class

Furthermore, a very basic unittest for this change was added.
Comment on lines 243 to 244
else:
raise AquaError('Please specify a driver as you instantiate OOVQE class.')
Copy link
Member

Choose a reason for hiding this comment

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

Since a driver can be passed in the constructor should this not be an additional test but now on self._driver and if not None raise an error. Although it seems you should always pass a driver in the constructor, i.e. its not optional there, and here it can be overridden so it should never be None here.

Copy link
Contributor

Choose a reason for hiding this comment

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

driver should only be passed to solve(...) not in constructor.

Copy link
Collaborator Author

@pbark pbark Oct 14, 2020

Choose a reason for hiding this comment

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

For the case of oovqe we need the driver to be passed also to the constructor in order to initialise the calculation and we pass the driver also to the solve(...) @Brogis1

Copy link
Contributor

@Brogis1 Brogis1 Oct 14, 2020

Choose a reason for hiding this comment

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

Exactly, I tried to avoid it but is necessary by this algorithm. There is no simple way of avoiding it for now.

Copy link
Contributor

@Brogis1 Brogis1 Oct 14, 2020

Choose a reason for hiding this comment

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

Since a driver can be passed in the constructor should this not be an additional test but now on self._driver and if not None raise an error. Although it seems you should always pass a driver in the constructor, i.e. its not optional there, and here it can be overridden so it should never be None here.

Yes, well spotted, it (raise AquaError..) can be removed. It remains from the time when I was trying not to use driver in the constructor of OOVQE.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can move all logic that depends on the driver to the solve function. You don't have to create all the things in init, just move them to the solve.

Copy link
Contributor

Choose a reason for hiding this comment

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

will require a minimum eigensolver factory. let's move to a separate PR and merge this one asap and have OOVQE included tomorrow.

Copy link
Contributor

@Brogis1 Brogis1 Oct 14, 2020

Choose a reason for hiding this comment

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

Ok it is movable (just tried) but like this only a part of OOVQE essential parameters will be initialized. 'solve' will initialize the rest when called. If you are ok with this, fine by me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I am fine with that!
I think that's how it should be, as you can then reuse it for different problems without having to adjust the algorithm itself.

Copy link
Contributor

@Brogis1 Brogis1 Oct 15, 2020

Choose a reason for hiding this comment

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

Great, moved the driver dependent logic to the solve function. Adapted the tests, all works. For the factory, we can sort out together with @mrossinek @adekusar-drl Work in progress in: https://github.com/pbark/qiskit-aqua/tree/oovqe

""" returns ground energy """
energies = self.get('eigenenergies')
if energies:
return energies[0].real
Copy link
Member

Choose a reason for hiding this comment

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

This condition was true for the excited state solvers that were/are here I think. In general it seems that this assumption could fail if the excited states algo does not include ground state. Just noting for future consideration rather than any change at present.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Excited states calculation should always include a ground state calculation. It is either bases on the Ground State Calculation (in EOM case) or the ground state is just one state of the states calculated by an exact eigensolver
(numpy)

Copy link
Member

Choose a reason for hiding this comment

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

You are citing what we have currently and yes today its true.

def filter_criterion(self, eigenstate, eigenvalue, aux_values):
# the first aux_value is the evaluated number of particles
num_particles_aux = aux_values[0][0]
return np.isclose(sum(self.molecule_info['num_particles']), num_particles_aux)
Copy link
Member

Choose a reason for hiding this comment

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

We ought to be able to match on the spin as well I think - particles is only one aspect. Particles plus spin should be the right filter I think.

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 agree with you. The reason we have the filter now is for the bosonic case and we need to measure only the number of particles (for this particular case). We can keep this as backlog I think for the upcoming development

Copy link
Member

Choose a reason for hiding this comment

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

It should be simple enough to fix this. I thought this was the for Numpy based Excited state solver on the Fermionic side where this filter arguably is needed too.

@woodsp-ibm
Copy link
Member

I will also note that this needs the overall Sphinx documentation sorted as to the navigation structure etc. Maybe its best to do this tomorrow after all the new chemistry pieces are in place.

manoelmarques and others added 2 commits October 14, 2020 18:30
…solver_factories/vqe_uccsd_factory.py

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
@manoelmarques manoelmarques dismissed mrossinek’s stale review October 14, 2020 23:05

Merge this PR and address any pending reviews in the next ot be created

@manoelmarques manoelmarques merged commit a8dbec9 into qiskit-community:master Oct 14, 2020
@adekusar-drl
Copy link
Contributor

Cool!

mtreinish pushed a commit to mtreinish/qiskit-core that referenced this pull request Nov 20, 2020
* ground state min eigensolver interface

* fix small things style

* added the molecule object with the right properties

* minor fix molecule

* excited state calculation added

* EOMvqe added

* ground state interface minimum eigensolver

* Add MinimumEigensolver interface to VQEAdapt

* Implement the AdaptVQEGroundStateCalculation class

This properly implements the AdaptVQEGroundStateCalculation class.
The idea of this class is to allow easy use of the VQEAdapt algorithm
which was previously extended to support the MinimumEigensolver
interface. This Calculation class is solely for the use with this
algorithm which is why it does not support setting a different solver.
Instead, it requires setting a QuantumInstance object through its
initializer.

The changes which this requires include the following:

- fixing the GroundStateCalculation class
- fixing the MinEigensolverGroundStateCalculation class

Furthermore, a very basic unittest for this change was added.

* comments

* initial bopes sampler

* naming

* compute eigenvalue MinEigensolver works

* return raw results until I make the GroundStateResult

* now the fermionic transformation returns a molecular ground state result working with mineigensolver result

* added TODOs

* rename GSC files

* add init to qubit_transformations

* fix lint in bosonic_trafo and qubit_op_trafo

* update (MES)GSC to new structure

* rm empty orbital optimization file

* Fix init

* Implement returns_groundstate and fix some lints

* Rewrite AdaptVQEGSCalc. deriving from MESGSCalc.

* Add TODO about deprecating qiskit.chemistry.algorithms module

* Revert "Add MinimumEigensolver interface to VQEAdapt"

This reverts commit 362843b19ff199e1b824b4318b14eee1d5c3da66.

* Properly deprecate VQEAdapt

* Port VQEAdapt to AdaptVQE (based on a GroundStateCalculation)

This properly refactors the old
qiskit.chemistry.algorithms.minimum_eigen_solver.VQEAdapt into the new
qiskit.chemistry.ground_state_calculation.AdaptVQE class which
implements the new GroundStateCalculation interface.

The name change is intentional because the new name is in-line with the
literature on AdaptVQE. The old name was presumably intentional to
highlight its relation to VQE.

The unittests for VQEAdapt are ported accordingly.

* Some minor corrections/updates

* Rename AdaptVQE files

* Add optional custom_excitation_pool argument to AdaptVQE computation method

* Work on FermionicTransformation and related objs

- add interpet method to QubitOperatorTransformation and
FermionicTransformation
- remove deprecated method `_process_..._deprecated`
- deprecate Hamiltonian and ChemistryOperator

* documentation added

* fixes in ground state

* patch MES result to GSC result instead of returning a tuple

* files fixes for GSC interface

* rm utf8 headers

* fix spell, lint, mypy

* Remove `custom_excitation_pool` from AdaptVQE `compute_ground_state` function

Rather than ignoring the arguments given by the interface, we can remove
this additional argument in favor of enabling the user to set this
custom excitation pool through the `MESFactory`.
An example on how to achieve such a thing can be seen in the
corresponding unittest.

* fix mypy

* fix lint

* Re-enable DeprecationWarnings after VQEAdapt has been initialized

* Fixes several minor nit-picks

- copyright years
- faulty renames
- faulty merge side-effects
- missing setters

* make MESFactory an interface only

* fix copyright years

* derive from MESFactory and update docs

* update docstrings and add molecule_info

* consistent naming in ``ground(_)state``

* fix spell

* more spell, deprecate MGSE

* fix deprecation

* more style

* test fixes

* fixes

* [WIP] basic Result-interfaces

This adds the following interfaces:
- qiskit.chemistry.ChemistryResult(AlgorithmResult)
- qiskit.chemistry.ground_state_calculation.GroundStateResult(ChemistryResult)
- qiskit.chemistry.ground_state_calculation.FermionicGroundStateResult(GroundStateResult)

The logic is as follows:
ChemistryResult and GroundStateResult are empty interfaces which define
a hierarchy for the result classes. They provide a useful granularity
for type checking. Once more subclasses are added we can extract common
functionality up into the stack as needed.

The FermionicGroundStateResult currently is just a port of the old
MolecularChemistryResult and MolecularGroundStateResult. Both of these
classes are deprecated in this commit. Since this class is currently
specific to the GroundStateCalculation it is part of that module. This
is, however, subject to change.

* Remove BosonicTransformation from this PR

The BosonicTransformation will be added in a separate PR aiming to
refactor the respective modules.

* AdaptVQE actually supports aux_operators

* [wip] resolve cyclic import problems

Having a `Result` class in either the `ground_state_calculations` or
`qubit_operator_transformations` modules leads to a coupling of the two
modules that is too tight. I.e. we run into cyclic imports whenever we
try to run any code because the `QubitOpTransformation` class needs to
load the `Result` class while at the same time it needs to be available
in the `GroundStateCalculation` class. Thus, when an additional coupling
in between these two modules is created, we have cyclic import.

Thus, we drop one level of granularity and only provide a single
result-interface on the `qiskit.chemistry` level, here, called
`ChemistryResult` (open for discussion). I also moved the `DipoleTuple`
to this level to simplify its usage in the other classes.

* Make AdaptVQE use the FermionicGSResult

* Use FermionicGroundStateResult in MinimumEigensolverGroundStateCalculation

* Fix mypy

* Filter DeprecationWarnings in unittests

This filters all of the DeprecationWarnigns introduced in this PR.
In 3 months from now we can remove these tests once the corresponding
code is also removed.

This commit also adds TODO labels to all of these unittest files which
will have to be migrated to work within the new framework.

* Add TODO in AdaptVQE unittest

* Add missing unittest imports

I noticed that in some test files the unittest module is not imported
and the `__main__` is not set accordingly. If we do not want to mix
these changes into this PR, we can simply revert this commit.

* mypy fix

* unused code

* Create qiskit.chemistry.results module

* Replace interpret() with add_context() in QubitOperatorTransformations

Instead of an interpret() function, the QubitOperatorTransformation
interface defines an add_context() function which is used to augment the
given StateResult object with information based on the transformation's
context.

* Fix GroundStateCalculation classes to work with the results interface

* Revert faulty change in github workflow

* unitests WIP first commit

* remove deprecation suppressing from new tests

* support additional aux ops and conv to dict

* fix aux_ops=None

* lint, style

* more test

* fixes in test

* fix style

* fix mypy

* fix mypy

* fixes in fermionic trafo

* remove editor created redundant file

* resolved tests

* docstring fix

* fix error & specify type hint

* change typehint Any -> FermionicOp

* add missing imports

* spell

* suggestions from code review

* consistent use in .keys()

* type hints, minor improvements

* Rethink the qiskit.chemistry.results

After some discussion we decided to rethink parts of the
qiskit.chemistry.results interfaces. We want to minimize the
differentiation between grround and excited states and let only the
concrete `QubitOperatorTransformation` handle the fermionic and bosonic
differentiation.

Thus, I apply the following changes:
* provide a single `EigenstateResult` which can be either a ground or
  excited state
* derive `ElectronicStructureResult` (prev. `FermionicResult`) from this
* use the `FermionicTransformation.interpret()` method to map a general
  `EigenstateResult` to the type specific for the transformation

With the last change we also align the `qiskit.chemistry` module further
with the `qiskit.optimization` module where we also use `interpret()`
rather than `add_context()` methods (which was the name we intended to
use prior to this commit).

* Remove unneeded DeprecationWarning from AdaptVQEResult

* Update some docstrings

* fix lint

* revert back to lists of aux_ops

* Update qiskit/chemistry/ground_state_calculation/adapt_vqe.py

Co-authored-by: Max Rossmannek <max.rossmannek@uzh.ch>

* apply suggestions from code review

* change typehint to List[float]

* Update qiskit/chemistry/core/hamiltonian.py

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>

* test fixes after code review

* Rename aux_values to aux_operator_eigenvalues

This naming is consistent with the EigensolverResult and
MinimumEigensolverResult classes.

* Remove leftover from dict-style aux_operators

* Add an `evaluate_operators` method to the GroundStateCalculation

This method can be used to evaluate any additional auxiliary operators
after the GSC has finished. This will be necessary especially in the
excited state calculations and is in general a nice option for the user
to have.

The method is similar to how the `VQE` evaluates auxiliary operators
internally. However, it can also deal with `MinimumEigensolver`s which
do not use a `QuantumInstance`.
A curiousity arose because the `VQE` appears to wrap the auxiliary
operator results into lists multiple times. In order to ensure
interoperability the same is done here.

The `AdaptVQE` now derives off of the `MinimumEigensolverGSC` because
there was no obvious drawback to this and the benefit of reusing this
new method was welcome.

Unittests have been added to test the new functionality.

* fix lint

* Remove erronous TODO

* Enforce a FermionicTransformation in the VQEUCCSDFactory

* Do not enforce VQEUCCSDFactory type in AdaptVQE

Since we check for the VQE and UCCSD types anyways we shouldn't force
the user to subclass this factory.

* Reuse the VQE object during AdaptVQE

* Expose molecule_info and qubit_mapping publicly

* fix lint

* fixes in tests

* fixes in tests and revert to molecule_methods

* remove two duplicate tests

* Expose some VQE arguments in the VQEUCCSDFactory

As discussed, we now expose some of the VQE arguments through this
factory in order to allow greater flexibility for the user.

* fix spell

* reverted due to missing part of the test uccsd_HF

* remove test EOM todos

* lint

* more lint

* more lint

* Prepare EigenstateResult to handle ground and excited states

* Deprecate the Enums in the chemistry.core module

* Also populate eigenstates in ground state calculations

* Add factory for NumPyMinimumEigensolver

* Implement `_filter_criterion` in FermionicTransformation

* Extract default filter_criterion into interface

Rather than checking for a private method being implemented we can
obtain the default filter_criterion cleanly.
This makes sense because any transformation in theory could implement a
default filter. However, we return None in case it is not implemented.

* Make use of default filter criterion configurable

* Check state's type before making at a StateFn

* Use `supports_aux_ops` in `returns_groundstate`

* Replace Any type-hint where not really necessary

The reason for making it a union of lists rather than a list of a union
is to ensure that no mixed lists are allowed.

* Do not use legacy Operator in new interface

The WeightedPauliOp is a legacy operator and should not be used in this
new interface. Instead, we now convert the operators to the OperatorFlow
before returning them to ensure we only use the new operator types.

* Except (Minimum)EigensolverResults in interpret()

To provide more user-flexibility and better compatibility with the Aqua
result classes, the interpret() method is able to handle
(Minimum)EigensolverResult classes as well as the EigenstateResult.

* Fix tests to work with OperatorBase rather than WeightedPauliOperator

* rm WPO support in trafo, add test for aux_ops

* Fix docstring

* Fix aux_op particle-hole conversion

* added reno

* initial OOVQE commit

* minor fixes

* fix style

* renaming

* typo in the properties

* fixes in OOVQE

* update reno

* update reno2

* update reno 3

* fix spell

* moving oovqe to a separate PR

* fix mypy

* Update qiskit/chemistry/algorithms/ground_state_solvers/minimum_eigensolver_factories/vqe_uccsd_factory.py

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>

* fix spell

Co-authored-by: Max Rossmannek <oss@zurich.ibm.com>
Co-authored-by: Stefan Woerner <WOR@zurich.ibm.com>
Co-authored-by: Cryoris <jules.gacon@googlemail.com>
Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: Manoel Marques <manoel.marques@ibm.com>
Co-authored-by: Anton Dekusar <adekusar@ie.ibm.com>
Co-authored-by: Anton Dekusar <62334182+adekusar-drl@users.noreply.github.com>
Co-authored-by: Max Rossmannek <max.rossmannek@uzh.ch>
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Co-authored-by: ISO <ISO@zurich.ibm.com>
Co-authored-by: Manoel Marques <manoelmrqs@gmail.com>
manoelmarques added a commit to manoelmarques/qiskit-terra that referenced this pull request Dec 7, 2020
* ground state min eigensolver interface

* fix small things style

* added the molecule object with the right properties

* minor fix molecule

* excited state calculation added

* EOMvqe added

* ground state interface minimum eigensolver

* Add MinimumEigensolver interface to VQEAdapt

* Implement the AdaptVQEGroundStateCalculation class

This properly implements the AdaptVQEGroundStateCalculation class.
The idea of this class is to allow easy use of the VQEAdapt algorithm
which was previously extended to support the MinimumEigensolver
interface. This Calculation class is solely for the use with this
algorithm which is why it does not support setting a different solver.
Instead, it requires setting a QuantumInstance object through its
initializer.

The changes which this requires include the following:

- fixing the GroundStateCalculation class
- fixing the MinEigensolverGroundStateCalculation class

Furthermore, a very basic unittest for this change was added.

* comments

* initial bopes sampler

* naming

* compute eigenvalue MinEigensolver works

* return raw results until I make the GroundStateResult

* now the fermionic transformation returns a molecular ground state result working with mineigensolver result

* added TODOs

* rename GSC files

* add init to qubit_transformations

* fix lint in bosonic_trafo and qubit_op_trafo

* update (MES)GSC to new structure

* rm empty orbital optimization file

* Fix init

* Implement returns_groundstate and fix some lints

* Rewrite AdaptVQEGSCalc. deriving from MESGSCalc.

* Add TODO about deprecating qiskit.chemistry.algorithms module

* Revert "Add MinimumEigensolver interface to VQEAdapt"

This reverts commit 362843b19ff199e1b824b4318b14eee1d5c3da66.

* Properly deprecate VQEAdapt

* Port VQEAdapt to AdaptVQE (based on a GroundStateCalculation)

This properly refactors the old
qiskit.chemistry.algorithms.minimum_eigen_solver.VQEAdapt into the new
qiskit.chemistry.ground_state_calculation.AdaptVQE class which
implements the new GroundStateCalculation interface.

The name change is intentional because the new name is in-line with the
literature on AdaptVQE. The old name was presumably intentional to
highlight its relation to VQE.

The unittests for VQEAdapt are ported accordingly.

* Some minor corrections/updates

* Rename AdaptVQE files

* Add optional custom_excitation_pool argument to AdaptVQE computation method

* Work on FermionicTransformation and related objs

- add interpet method to QubitOperatorTransformation and
FermionicTransformation
- remove deprecated method `_process_..._deprecated`
- deprecate Hamiltonian and ChemistryOperator

* documentation added

* fixes in ground state

* patch MES result to GSC result instead of returning a tuple

* files fixes for GSC interface

* rm utf8 headers

* fix spell, lint, mypy

* Remove `custom_excitation_pool` from AdaptVQE `compute_ground_state` function

Rather than ignoring the arguments given by the interface, we can remove
this additional argument in favor of enabling the user to set this
custom excitation pool through the `MESFactory`.
An example on how to achieve such a thing can be seen in the
corresponding unittest.

* fix mypy

* fix lint

* Re-enable DeprecationWarnings after VQEAdapt has been initialized

* Fixes several minor nit-picks

- copyright years
- faulty renames
- faulty merge side-effects
- missing setters

* make MESFactory an interface only

* fix copyright years

* derive from MESFactory and update docs

* update docstrings and add molecule_info

* consistent naming in ``ground(_)state``

* fix spell

* more spell, deprecate MGSE

* fix deprecation

* more style

* test fixes

* fixes

* [WIP] basic Result-interfaces

This adds the following interfaces:
- qiskit.chemistry.ChemistryResult(AlgorithmResult)
- qiskit.chemistry.ground_state_calculation.GroundStateResult(ChemistryResult)
- qiskit.chemistry.ground_state_calculation.FermionicGroundStateResult(GroundStateResult)

The logic is as follows:
ChemistryResult and GroundStateResult are empty interfaces which define
a hierarchy for the result classes. They provide a useful granularity
for type checking. Once more subclasses are added we can extract common
functionality up into the stack as needed.

The FermionicGroundStateResult currently is just a port of the old
MolecularChemistryResult and MolecularGroundStateResult. Both of these
classes are deprecated in this commit. Since this class is currently
specific to the GroundStateCalculation it is part of that module. This
is, however, subject to change.

* Remove BosonicTransformation from this PR

The BosonicTransformation will be added in a separate PR aiming to
refactor the respective modules.

* AdaptVQE actually supports aux_operators

* [wip] resolve cyclic import problems

Having a `Result` class in either the `ground_state_calculations` or
`qubit_operator_transformations` modules leads to a coupling of the two
modules that is too tight. I.e. we run into cyclic imports whenever we
try to run any code because the `QubitOpTransformation` class needs to
load the `Result` class while at the same time it needs to be available
in the `GroundStateCalculation` class. Thus, when an additional coupling
in between these two modules is created, we have cyclic import.

Thus, we drop one level of granularity and only provide a single
result-interface on the `qiskit.chemistry` level, here, called
`ChemistryResult` (open for discussion). I also moved the `DipoleTuple`
to this level to simplify its usage in the other classes.

* Make AdaptVQE use the FermionicGSResult

* Use FermionicGroundStateResult in MinimumEigensolverGroundStateCalculation

* Fix mypy

* Filter DeprecationWarnings in unittests

This filters all of the DeprecationWarnigns introduced in this PR.
In 3 months from now we can remove these tests once the corresponding
code is also removed.

This commit also adds TODO labels to all of these unittest files which
will have to be migrated to work within the new framework.

* Add TODO in AdaptVQE unittest

* Add missing unittest imports

I noticed that in some test files the unittest module is not imported
and the `__main__` is not set accordingly. If we do not want to mix
these changes into this PR, we can simply revert this commit.

* mypy fix

* unused code

* Create qiskit.chemistry.results module

* Replace interpret() with add_context() in QubitOperatorTransformations

Instead of an interpret() function, the QubitOperatorTransformation
interface defines an add_context() function which is used to augment the
given StateResult object with information based on the transformation's
context.

* Fix GroundStateCalculation classes to work with the results interface

* Revert faulty change in github workflow

* unitests WIP first commit

* remove deprecation suppressing from new tests

* support additional aux ops and conv to dict

* fix aux_ops=None

* lint, style

* more test

* fixes in test

* fix style

* fix mypy

* fix mypy

* fixes in fermionic trafo

* remove editor created redundant file

* resolved tests

* docstring fix

* fix error & specify type hint

* change typehint Any -> FermionicOp

* add missing imports

* spell

* suggestions from code review

* consistent use in .keys()

* type hints, minor improvements

* Rethink the qiskit.chemistry.results

After some discussion we decided to rethink parts of the
qiskit.chemistry.results interfaces. We want to minimize the
differentiation between grround and excited states and let only the
concrete `QubitOperatorTransformation` handle the fermionic and bosonic
differentiation.

Thus, I apply the following changes:
* provide a single `EigenstateResult` which can be either a ground or
  excited state
* derive `ElectronicStructureResult` (prev. `FermionicResult`) from this
* use the `FermionicTransformation.interpret()` method to map a general
  `EigenstateResult` to the type specific for the transformation

With the last change we also align the `qiskit.chemistry` module further
with the `qiskit.optimization` module where we also use `interpret()`
rather than `add_context()` methods (which was the name we intended to
use prior to this commit).

* Remove unneeded DeprecationWarning from AdaptVQEResult

* Update some docstrings

* fix lint

* revert back to lists of aux_ops

* Update qiskit/chemistry/ground_state_calculation/adapt_vqe.py

Co-authored-by: Max Rossmannek <max.rossmannek@uzh.ch>

* apply suggestions from code review

* change typehint to List[float]

* Update qiskit/chemistry/core/hamiltonian.py

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>

* test fixes after code review

* Rename aux_values to aux_operator_eigenvalues

This naming is consistent with the EigensolverResult and
MinimumEigensolverResult classes.

* Remove leftover from dict-style aux_operators

* Add an `evaluate_operators` method to the GroundStateCalculation

This method can be used to evaluate any additional auxiliary operators
after the GSC has finished. This will be necessary especially in the
excited state calculations and is in general a nice option for the user
to have.

The method is similar to how the `VQE` evaluates auxiliary operators
internally. However, it can also deal with `MinimumEigensolver`s which
do not use a `QuantumInstance`.
A curiousity arose because the `VQE` appears to wrap the auxiliary
operator results into lists multiple times. In order to ensure
interoperability the same is done here.

The `AdaptVQE` now derives off of the `MinimumEigensolverGSC` because
there was no obvious drawback to this and the benefit of reusing this
new method was welcome.

Unittests have been added to test the new functionality.

* fix lint

* Remove erronous TODO

* Enforce a FermionicTransformation in the VQEUCCSDFactory

* Do not enforce VQEUCCSDFactory type in AdaptVQE

Since we check for the VQE and UCCSD types anyways we shouldn't force
the user to subclass this factory.

* Reuse the VQE object during AdaptVQE

* Expose molecule_info and qubit_mapping publicly

* fix lint

* fixes in tests

* fixes in tests and revert to molecule_methods

* remove two duplicate tests

* Expose some VQE arguments in the VQEUCCSDFactory

As discussed, we now expose some of the VQE arguments through this
factory in order to allow greater flexibility for the user.

* fix spell

* reverted due to missing part of the test uccsd_HF

* remove test EOM todos

* lint

* more lint

* more lint

* Prepare EigenstateResult to handle ground and excited states

* Deprecate the Enums in the chemistry.core module

* Also populate eigenstates in ground state calculations

* Add factory for NumPyMinimumEigensolver

* Implement `_filter_criterion` in FermionicTransformation

* Extract default filter_criterion into interface

Rather than checking for a private method being implemented we can
obtain the default filter_criterion cleanly.
This makes sense because any transformation in theory could implement a
default filter. However, we return None in case it is not implemented.

* Make use of default filter criterion configurable

* Check state's type before making at a StateFn

* Use `supports_aux_ops` in `returns_groundstate`

* Replace Any type-hint where not really necessary

The reason for making it a union of lists rather than a list of a union
is to ensure that no mixed lists are allowed.

* Do not use legacy Operator in new interface

The WeightedPauliOp is a legacy operator and should not be used in this
new interface. Instead, we now convert the operators to the OperatorFlow
before returning them to ensure we only use the new operator types.

* Except (Minimum)EigensolverResults in interpret()

To provide more user-flexibility and better compatibility with the Aqua
result classes, the interpret() method is able to handle
(Minimum)EigensolverResult classes as well as the EigenstateResult.

* Fix tests to work with OperatorBase rather than WeightedPauliOperator

* rm WPO support in trafo, add test for aux_ops

* Fix docstring

* Fix aux_op particle-hole conversion

* added reno

* initial OOVQE commit

* minor fixes

* fix style

* renaming

* typo in the properties

* fixes in OOVQE

* update reno

* update reno2

* update reno 3

* fix spell

* moving oovqe to a separate PR

* fix mypy

* Update qiskit/chemistry/algorithms/ground_state_solvers/minimum_eigensolver_factories/vqe_uccsd_factory.py

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>

* fix spell

Co-authored-by: Max Rossmannek <oss@zurich.ibm.com>
Co-authored-by: Stefan Woerner <WOR@zurich.ibm.com>
Co-authored-by: Cryoris <jules.gacon@googlemail.com>
Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: Manoel Marques <manoel.marques@ibm.com>
Co-authored-by: Anton Dekusar <adekusar@ie.ibm.com>
Co-authored-by: Anton Dekusar <62334182+adekusar-drl@users.noreply.github.com>
Co-authored-by: Max Rossmannek <max.rossmannek@uzh.ch>
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Co-authored-by: ISO <ISO@zurich.ibm.com>
Co-authored-by: Manoel Marques <manoelmrqs@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Changelog: API Change Include in the Changed section of the changelog Changelog: Deprecation Include in Deprecated section of changelog Changelog: New Feature Include in the Added section of the changelog Chemistry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants