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.

Sign up for GitHub

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

Jump to bottom

QEOM energies differ for Statevector and Qasm simulator #52

Closed
MariaSapova opened this issue Nov 30, 2020 · 12 comments · Fixed by #971
Closed

QEOM energies differ for Statevector and Qasm simulator #52

MariaSapova opened this issue Nov 30, 2020 · 12 comments · Fixed by #971
Labels
status: confirmed type: bug Something isn't working

Comments

@MariaSapova
Copy link

Information

  • Qiskit Aqua version: 0.8.1
  • Python version: 3.6.8
  • Operating system: Ubuntu

What is the current behavior?

For H2 molecule QEOM excited state energies differ running with Statevector and Qasm simulators.

Steps to reproduce the problem

Please, run the gist

What is the expected behavior?

Close excited state energies

Suggested solutions

Seems like the problem is somewhere in eval() method, but I'm not sure about that.
@ikkoham
Copy link
Contributor

ikkoham commented Dec 1, 2020

How about using Aer for statevector simulator instead of BasicAer?

@MariaSapova
Copy link
Author

@ikkoham Hello! Aer for statevector simulator provides the same results as BasicAer and they are equal to exact diagonalization results. But still there is some problem with qasm.

@yaelbh
Copy link

yaelbh commented Dec 1, 2020

Qasm by default performs measurements, so you obtain statistical errors. This is its default behavior, because users often use it to mimic a real device. If you want exact diagonalization, try setting the parameter initial_custom to True.

@MariaSapova
Copy link
Author

Hello @yaelbh!
I use include_custom = True with qasm simulator.
Still excited state energies deviate greatly.
image
And also there is some obvious error in matrix element calculations.
image

mrossinek referenced this issue in mrossinek/qiskit-aqua Dec 4, 2020
@mrossinek
Ensure aux_operator eigenvalues are normalized
ffcb71a 
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.
mrossinek referenced this issue in mrossinek/qiskit-aqua Dec 4, 2020
@mrossinek @Cryoris
Ensure aux_operator eigenvalues are normalized
e72216e 
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 referenced this issue in mrossinek/qiskit Dec 9, 2020
@mrossinek @Cryoris
Ensure aux_operator eigenvalues are normalized
49099e2 
As reported on multiple occasions in Qiskit Aqua ([1], [2]), 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.

A unittest is added which asserts this behavior.

[1]: https://github.com/Qiskit/qiskit-aqua/issues/1460
[2]: qiskit-community/qiskit-aqua#1467

Co-authored-by: Julien Gacon <gaconju@gmail.com>
@MariaSapova
Copy link
Author

Hello @paulineollitrault ! Could you please provide any insights on how to deal with this inconsistence in qasm results?

@paulineollitrault
Copy link
Contributor

Hi @MariaSapova! A fix is pending (see above), waiting to be merged

@MariaSapova
Copy link
Author

MariaSapova commented Dec 21, 2020 •
edited
Loading

@paulineollitrault as far as I can see this commit fixes the normalization of the matrix elements, but what about differences in q_0_0, q_1_1, q_2_2 values? I guess the energies differ because of these matrix elements.

image

mergify bot referenced this issue in Qiskit/qiskit Dec 28, 2020
@mrossinek @Cryoris @mergify
Ensure aux_operator eigenvalues are normalized (#5496)
4d3ed6a 
As reported on multiple occasions in Qiskit Aqua ([1], [2]), 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.

A unittest is added which asserts this behavior.

[1]: https://github.com/Qiskit/qiskit-aqua/issues/1460
[2]: qiskit-community/qiskit-aqua#1467

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

Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
molar-volume referenced this issue in molar-volume/qiskit-terra Dec 29, 2020
@mrossinek @Cryoris
@mergify @molar-volume
Ensure aux_operator eigenvalues are normalized (Qiskit#5496)
4e8a076 
As reported on multiple occasions in Qiskit Aqua ([1], [2]), 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.

A unittest is added which asserts this behavior.

[1]: https://github.com/Qiskit/qiskit-aqua/issues/1460
[2]: qiskit-community/qiskit-aqua#1467

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

Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
mergify bot referenced this issue in Qiskit/qiskit Jan 13, 2021
@molar-volume @Cryoris
@kdk @anedumla @mrossinek @mergify
Improved handling of params argument in Gradient.convert (#5548)
799ad14 
* 1. params are required in convert method in GradientBased and HessianBased subclasses
2. fix on handling params of type ParameterExpression

* Update qiskit/opflow/gradients/gradient.py

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

* passmanager(..., callback=...) parameter removed (#5522)

* passmanager callback removal

* unused-import

* reno

* better initial_layout validation (#5526)

Co-authored-by: Kevin Krsulich <kevin.krsulich@ibm.com>

* 1) unit tests modified to test ParameterVector input for Gradient, NaturalGradient, QFI and Hessian
2) index method added to ParameterVector
3) handling of ParameterVector changed for Hessian to be consistent with list params

* Hessian with respect to vector-like params returns matrix, unit test modified accordingly

* Add PiecewiseChebyshev arithmetic circuit (#5364)

* general polynomial approximation

* release notes

* fix qubit re-allocation

* fix re-calling build

* Update qiskit/circuit/library/arithmetic/piecewise_chebyshev.py

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

* Update qiskit/circuit/library/arithmetic/piecewise_chebyshev.py

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

* Update qiskit/circuit/library/arithmetic/piecewise_chebyshev.py

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

* fix indentation lint error

* move tests to library module

* Add suggestions from code review

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

* Ensure aux_operator eigenvalues are normalized (#5496)

As reported on multiple occasions in Qiskit Aqua ([1], [2]), 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.

A unittest is added which asserts this behavior.

[1]: https://github.com/Qiskit/qiskit-aqua/issues/1460
[2]: qiskit-community/qiskit-aqua#1467

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

Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* Hessian with respect to vector-like param returns matrix, unit test modified accordingly

Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: Luciano Bello <luciano.bello@ibm.com>
Co-authored-by: Kevin Krsulich <kevin.krsulich@ibm.com>
Co-authored-by: Almudena Carrera Vazquez <almudenacarreravazquez@hotmail.com>
Co-authored-by: Max Rossmannek <oss@zurich.ibm.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
@woodsp-ibm
Copy link
Member

@paulineollitrault Do you have any response to the last comment above that despite the normalization fix being merged there are still differences? If this is still an issue I will transfer this to the new repo to be fixed there/

@MariaSapova
Copy link
Author

@woodsp-ibm @paulineollitrault Hello! I actually already opened another issue which is more general Qiskit/qiskit-aqua#1510. I found out that evaluate_operators() in GroundStateEigensolvers produces incorrect expectation values when using qasm. With rewriting expectation value evaluation I could overcome this issue and got correct matricees in QEOM.

@woodsp-ibm woodsp-ibm transferred this issue from qiskit-community/qiskit-aqua Feb 16, 2021
@MariaSapova MariaSapova mentioned this issue Feb 16, 2021
Closed
@mrossinek mrossinek mentioned this issue Mar 31, 2021
Merged
@mrossinek mrossinek added status: confirmed type: bug Something isn't working labels Mar 31, 2021
@mrossinek
Copy link
Member

I investigated this bug together with @Cryoris again today but we have not been able to track down the exact cause of it yet.. We opened a PR to fix a sub-issue of this but I am afraid a fix for this issue will not make it into the upcoming release.
I will revisit this again in more detail later.

@mrossinek mrossinek mentioned this issue Nov 5, 2021
Closed
Anthony-Gandon pushed a commit to Anthony-Gandon/qiskit-nature that referenced this issue Apr 1, 2022
@Anthony-Gandon
Fix the incorrect results given by QEOM with noisy simulators
736c34a 
Fixes qiskit-community#52
Fixes qiskit-community#415

Previously, QEOM evaluated the many operators needed to create the matrices Q,M on the
groundstate resulting from a call to VQE. This worked for StateVector simulators because
this object was exactly the statefunction (type VectorStateFn). However, when using QASM or
aer_simulator this eigenstate is a dictionary of samples which was converted
into a VectorStateFn without consideration of the sign ambiguity.

This commit changes the evaluate_operators() method used in QEOM to the eval_observables()
that was added in a recent commit of qiskit-terra.
This method is called on the ansatz function evaluated at the optimal parameters given by
the solver instead of on the groundstate. This delays the call to the eval() method which
was the root of the issue.

Added the required support for non-hermitian operators in the aer_pauli_expectation class
@Anthony-Gandon Anthony-Gandon mentioned this issue Apr 1, 2022
Closed
This was referenced Nov 15, 2022
Closed
Closed
Merged
@Anthony-Gandon
Copy link
Contributor

This issue seems to have been solved as a side effect of the new Qiskit release.
The problematic methods for this issue (eval_op() and evaluate_operators()) have been deprecated and replaced.

The new behaviour of the QEOM calculation does not show this difference between the noisy and state vector "backend" (this has also been replaced): see this gist

@mrossinek
Copy link
Member

Thank you, Anthony! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
status: confirmed type: bug Something isn't working
Projects
None yet
Milestone
No milestone
7 participants
@ikkoham @mrossinek @yaelbh @Anthony-Gandon @woodsp-ibm @paulineollitrault @MariaSapova