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

Equality checks fail for EstimatorResult objects #9464

Closed
pedrorrivero opened this issue Jan 25, 2023 · 5 comments
Closed

Equality checks fail for EstimatorResult objects #9464

pedrorrivero opened this issue Jan 25, 2023 · 5 comments
Labels
bug Something isn't working mod: primitives Related to the Primitives module

Comments

@pedrorrivero
Copy link
Member

Environment

  • Qiskit Terra version: 0.22.2
  • Python version: 3.11
  • Operating system: MacOS

What is happening?

Equality checks fail in EstimatorResult objects because values attr is np.array and == is not well behaved for those.

How can we reproduce the issue?

from numpy import array
from qiskit.primitives import EstimatorResult

values = list(range(9))
metadata = [{} for _ in values]

result_1 = EstimatorResult(values=array(values), metadata=metadata)
result_2 = EstimatorResult(values=array(values), metadata=metadata)

assert result_1 == result_2

Outputs:

---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Cell In [8], line 10
      7 result_1 = EstimatorResult(values=array(values), metadata=metadata)
      8 result_2 = EstimatorResult(values=array(values), metadata=metadata)
---> 10 assert result_1 == result_2
     11 result_1

File <string>:4, in __eq__(self, other)

ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

What should happen?

Equality should return True or False appropriately.

Any suggestions?

We should move to using tuple (i.e. immutable, core python types) instead or np.array.

@pedrorrivero pedrorrivero added the bug Something isn't working label Jan 25, 2023
@t-imamichi
Copy link
Member

t-imamichi commented Jan 26, 2023

It's not a good idea to check equality of floating-point values with ==.
There is a simple example in the official documentation https://docs.python.org/3.11/tutorial/floatingpoint.html.

print(0.1 + 0.1 + 0.1 == 0.3)
# False

I suggest np.allclose for the comparison of EstimatorResult.values.
https://numpy.org/doc/stable/reference/generated/numpy.allclose.html

So, it might be an option to set dataclass(eq=False) to clarify that == of EstimatorRestult is not recommended.

@pedrorrivero
Copy link
Member Author

That is a good point @t-imamichi.

It might be even better to provide a custom __eq__ method in the class, overriding the default one for dataclass.

Nonetheless, I have to insist in moving away from numpy types though. The values field is not even a vector per-se, but a collection of floats, and issues like this will keep coming up.

@t-imamichi
Copy link
Member

t-imamichi commented Jan 26, 2023

I prefer introducing a utility function of the equality check based on np.allclose.
assert result_1 == result_2 easily fails due to the floating-point numbers even if values is tuple.
I think we need to allow end users configure atol and/or rtol for the equality check.

I don't have a strong opinion of the type of values itself. I'm fine to change it from ndarray to tuple.

@ikkoham
Copy link
Contributor

ikkoham commented Jan 26, 2023

My recommendation is eq=False in dataclass.

custom __eq__ method in the class

I prefer introducing a utility function of the equality check based on np.allclose.

Do these even compare metadata content?

@woodsp-ibm woodsp-ibm added the mod: primitives Related to the Primitives module label Jan 26, 2023
@t-imamichi
Copy link
Member

I close this because the Primitives V1 is deprecated #12575

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working mod: primitives Related to the Primitives module
Projects
None yet
Development

No branches or pull requests

4 participants