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 evaluation of additional operators #5

Merged
merged 14 commits into from
Oct 8, 2020

Conversation

Cryoris
Copy link
Collaborator

@Cryoris Cryoris commented Oct 7, 2020

Summary

Add the possibility to pass a dictionary of auxiliary operators to compute_groundstate and transform.

Details and comments

  • changed the data structure of aux operators from a list to a dictionary: {str: FermionicOperator/BosonicOperator}
  • the type hint in cases where fermionic and bosonic is supported is Any but could possible be Union[FermionicOperator, BosonicOperator] once we have the bosonic operator
  • fixed a bug in adapt VQE where the values of the aux operators were not stored

Open questions

  1. The MinimumEigensolver interface also supports aux operators but there they are a list. Since this is a mathematically abstract solver w/o any interpretation it might make sense to keep them as list there. This is what's done now.
  2. Are we breaking backwards compatibility somewhere by changing list->dict?

@Cryoris Cryoris requested review from mrossinek and stefan-woerner and removed request for manoelmarques and woodsp-ibm October 7, 2020 09:14
Copy link
Collaborator

@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.

I left some minor comments.
I can't think of any place where we would need to add a DeprecationWarning for the type change from list to dict. But maybe the others can think of something.

@pbark
Copy link
Owner

pbark commented Oct 7, 2020

I think that now it looks ok. Should I merge it or are there any other comments?

Copy link
Collaborator

@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.

There are some leftovers from the dict-style aux_ops.

qiskit/chemistry/ground_state_calculation/adapt_vqe.py Outdated Show resolved Hide resolved
qiskit/chemistry/results/eigenstate_result.py Outdated Show resolved Hide resolved
qiskit/chemistry/results/eigenstate_result.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

lgtm 👍

@pbark pbark merged commit a15326d into pbark:ground_state_interface Oct 8, 2020
@Cryoris Cryoris deleted the aux_operators branch October 8, 2020 08:10
@woodsp-ibm
Copy link
Collaborator

I think the aux op values, like the eigen values - were always complex numbers so far

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.

4 participants