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

Ensure aux_operator eigenvalues are normalized#1473

Closed
mrossinek wants to merge 2 commits intoqiskit-community:masterfrom
mrossinek:ensure-normalized-aux-op-values
Closed

Ensure aux_operator eigenvalues are normalized#1473
mrossinek wants to merge 2 commits intoqiskit-community:masterfrom
mrossinek:ensure-normalized-aux-op-values

Conversation

@mrossinek
Copy link
Copy Markdown
Member

Summary

As reported on multiple occasions by @MariaSapova (#1460 and #1467), the eigenvalues of
the auxiliary operators are not correctly normalized when the QASM
backend is used.

This commit fixes this short-coming by applying the same normalization
to the VQE's eigenstate when obtained from a QASM backend (in which case
this is a dictionary) as done by the CircuitSampler in its
sample_circuits function.
The case of the statevector backend is unaffected by this change, as it
will return the eigenstate as a list.

As reported on multiple occasions (#1460 and qiskit-community#1467), the eigenvalues of
the auxiliary operators are not correctly normalized when the QASM
backend is used.

This commit fixes this short-coming by applying the same normalization
to the VQE's eigenstate when obtained from a QASM backend (in which case
this is a dictionary) as done by the `CircuitSampler` in its
`sample_circuits` function.
The case of the statevector backend is unaffected by this change, as it
will return the eigenstate as a list.

Co-authored-by: Julien Gacon <gaconju@gmail.com>
@mrossinek

This comment has been minimized.

@mrossinek mrossinek force-pushed the ensure-normalized-aux-op-values branch from ffcb71a to e72216e Compare December 4, 2020 17:14
@Cryoris Cryoris linked an issue Dec 5, 2020 that may be closed by this pull request
Cryoris
Cryoris previously approved these changes Dec 5, 2020
@ikkoham
Copy link
Copy Markdown
Contributor

ikkoham commented Dec 7, 2020

Hello, this fix sounds nice, but why didn't you add this processsing here?

@mrossinek
Copy link
Copy Markdown
Member Author

Hi @ikkoham! I am not sure to be honest.. but that does indeed look like a better place to fit this. I will update the PR now.

@Cryoris
Copy link
Copy Markdown
Contributor

Cryoris commented Dec 7, 2020

We actually thought of putting it there in the first place -- but the reason we didn't is to not change the current behavior that get_optimal_vector gives you the circuit output. Personally, I'd be good with changing it there but maybe @pbark @woodsp-ibm or @manoelmarques have an opinion here

@mrossinek
Copy link
Copy Markdown
Member Author

We actually thought of putting it there in the first place -- but the reason we didn't is to not change the current behavior that get_optimal_vector gives you the circuit output.

Ah I didn't remember correctly then.. well I agree though that this makes more sense even though it changes the current behavior. The reason being that I think it improves the current behavior..

@Cryoris
Copy link
Copy Markdown
Contributor

Cryoris commented Dec 8, 2020

I'm good with changing this at the higher level. We'll have to wait for the merge of the algos to Terra though and then do the change there.

@woodsp-ibm
Copy link
Copy Markdown
Member

I feel there ought to be a unit test around this - maybe augment one of those that exist. Just wondering was this specific to VQE-Adapt. I do not recall any issue with VQE in the past.

@mrossinek
Copy link
Copy Markdown
Member Author

Yes a unittest should be added. I will look into that once we re-open this PR when everything is merged into Terra.

Just wondering was this specific to VQE-Adapt
@woodsp-ibm This was reported both, for AdaptVQE (#1467) and for QEOM (#1460)

I also never noticed this before but I am not sure when this broke. I tried bisecting it but I didn't have enough time to go through with this because I would have to adapt the test script a lot because of the many refactorings we did lately (i.e. the test script to check whether it breaks needs to be changed).
I might look into this in more detail once revisiting the PR for terra.

@mrossinek
Copy link
Copy Markdown
Member Author

I migrated this PR over to Terra and also added a unittest while I was at it.
Closing this PR.

@mrossinek mrossinek closed this Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ADAPT-VQE fails with Aer simulator

4 participants