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

SymbolicOperator.isclose bug for operators with many terms? #764

Open
peterse opened this issue Jan 23, 2022 · 0 comments
Open

SymbolicOperator.isclose bug for operators with many terms? #764

peterse opened this issue Jan 23, 2022 · 0 comments

Comments

@peterse
Copy link

peterse commented Jan 23, 2022

the following lines of code (v1.3.0 release branch) implementing SymbolicOperator.isclose are doing something strange when comparing two operators:

if not (isinstance(a, sympy.Expr) or isinstance(b, sympy.Expr)):
    tol *= max(1, abs(a), abs(b))

This behavior means that operators x and y whose terms have large coefficients will satisfy x == y even when x - y > 0. Here is a minimal reproducing example

from openfermion import FermionOperator
x = FermionOperator("0^ 0")
y = FermionOperator("0^ 0")

# construct two identical operators up to some number of terms
num_terms_before_ineq = 30
for i in range(num_terms_before_ineq):
    x += FermionOperator(f" (10+0j) [0^ {i}]")
    y += FermionOperator(f" (10+0j) [0^ {i}]")
    
# add a final term that is equal within tol but gets missed by the isclose check
xfinal = FermionOperator(f" (1+0j) [0^ {num_terms_before_ineq + 1}]")
yfinal = FermionOperator(f" (2+0j) [0^ {num_terms_before_ineq + 1}]")
# these two terms are not equal within tol..
print(xfinal == yfinal) # >>> False
print(xfinal - yfinal) # (-1+0j) [0^ 31]

# ...but these two terms will be, because the `tol` argument to `isclose` was corrupted
x += xfinal
y += yfinal
print(x == y) # >>> True
print(x - y) # (-1+0j) [0^ 31]

you can tweak num_terms_before_ineq smaller and the output will correctly reflect that x and y are not equal within the desired tolerance.

Otherwise if this is the intended behavior (i.e. this comparison is meant to be equality within a relative tolerance w/r to magnitude of the coefficient vectors for x and y), the docstring for SymbolicOperator.isclose should be updated accordingly.

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

No branches or pull requests

1 participant