Fix number comparisons vs expression comparisons (#1521)

This PR improves the compatibility with the different WMA sorting
schemes for numbers and expressions.

* Now the sort key for Complex is the concatenation of the sort keys for the real and the imaginary part.
* Sort keys for other numbers were tuned to improve the compatibility with WMA
* `__eq__` methods for numbers do not rely on sort keys but on numeric equivalence. 
* Adding low-level tests for canonical order in numbers.
---------

Co-authored-by: mmatera <[email protected]>

Author: mmatera

mathics/core/atoms.py

@@ -5,7 +5,7 @@
 import math
 import re
 from functools import cache
-from typing import Any, Dict, Generic, Optional, Tuple, TypeVar, Union
+from typing import Any, Dict, Generic, Optional, Tuple, TypeVar, Union, cast
 
 import mpmath
 import numpy
@@ -239,11 +239,14 @@ def __new__(cls, value) -> "Integer":
         return self
 
     def __eq__(self, other) -> bool:
-        return (
-            self._value == other.value
-            if isinstance(other, Integer)
-            else super().__eq__(other)
-        )
+        if isinstance(other, Integer):
+            return self._value == other._value
+        if isinstance(other, Number):
+            # If other is a number of a wider class, use
+            # its implementation:
+            return other.__eq__(self)
+
+        return super().__eq__(other)
 
     def __ge__(self, other) -> bool:
         return (
@@ -346,7 +349,7 @@ def to_sympy(self, **_) -> sympy_numbers.Integer:
 
     def sameQ(self, rhs) -> bool:
         """Mathics SameQ"""
-        return isinstance(rhs, Integer) and self._value == rhs.value
+        return isinstance(rhs, Integer) and self._value == rhs._value
 
     def do_copy(self) -> "Integer":
         return Integer(self._value)
@@ -404,17 +407,18 @@ def __new__(cls, value, p: Optional[int] = None) -> "Real":
             return PrecisionReal.__new__(PrecisionReal, value)
 
     def __eq__(self, other) -> bool:
-        if isinstance(other, Real):
-            # MMA Docs: "Approximate numbers that differ in their last seven
-            # binary digits are considered equal"
-            _prec = min_prec(self, other)
-            if _prec is not None:
-                with mpmath.workprec(_prec):
-                    rel_eps = 0.5 ** float(_prec - 7)
-                    return mpmath.almosteq(
-                        self.to_mpmath(), other.to_mpmath(), abs_eps=0, rel_eps=rel_eps
-                    )
-        return super().__eq__(other)
+        if not isinstance(other, Number):
+            return super().__eq__(other)
+
+        _prec: Optional[int] = min_prec(self, other)
+        if _prec is None:
+            return self._value == other._value
+
+        with mpmath.workprec(_prec):
+            rel_eps = 0.5 ** float(_prec - 7)
+            return mpmath.almosteq(
+                self.to_mpmath(), other.to_mpmath(), abs_eps=0, rel_eps=rel_eps
+            )
 
     def __hash__(self):
         # ignore last 7 binary digits when hashing
@@ -492,6 +496,20 @@ def get_precision(self) -> int:
     def get_float_value(self, permit_complex=False) -> float:
         return self._value
 
+    @property
+    def element_order(self) -> tuple:
+        """
+        Return a tuple value that is used in ordering elements
+        of an expression. The tuple is ultimately compared lexicographically.
+        """
+        return (
+            BASIC_ATOM_NUMBER_ELT_ORDER,
+            self._value,
+            0,
+            1,
+            0,  # Machine precision comes first, and after Integers
+        )
+
     @property
     def is_approx_zero(self) -> bool:
         # In WMA, Chop[10.^(-10)] == 0,
@@ -514,7 +532,7 @@ def make_boxes(self, form):
 
     @property
     def is_zero(self) -> bool:
-        return self.value == 0.0
+        return self._value == 0.0
 
     def sameQ(self, rhs) -> bool:
         """Mathics SameQ for MachineReal.
@@ -524,9 +542,9 @@ def sameQ(self, rhs) -> bool:
         rhs-value's precision.  For any rhs type, sameQ is False.
         """
         if isinstance(rhs, MachineReal):
-            return self.value == rhs.value
+            return self._value == rhs._value
         if isinstance(rhs, PrecisionReal):
-            rhs_value = rhs.value
+            rhs_value = rhs._value
             value = self.to_sympy()
             # If sympy fixes the issue, this comparison would be
             # enough
@@ -603,6 +621,21 @@ def get_precision(self) -> int:
         """Returns the default specification for precision (in binary digits) in N and other numerical functions."""
         return self.value._prec + 1
 
+    @property
+    def element_order(self) -> tuple:
+        """
+        Return a tuple value that is used in ordering elements
+        of an expression. The tuple is ultimately compared lexicographically.
+        """
+
+        value = self._value
+        value, prec = float(value), value._prec
+        # For large values, use the sympy.Float value...
+        if math.isinf(value):
+            value, prec = self._value, value._prec
+
+        return (BASIC_ATOM_NUMBER_ELT_ORDER, value, 0, 2, prec)
+
     @property
     def is_zero(self) -> bool:
         # self.value == 0 does not work for sympy >=1.13
@@ -757,7 +790,7 @@ def sameQ(self, rhs) -> bool:
         """Mathics3 SameQ"""
         # FIX: check
         if isinstance(rhs, ByteArray):
-            return self.value == rhs.value
+            return self._value == rhs._value
         return False
 
     def get_string_value(self) -> Optional[str]:
@@ -902,12 +935,15 @@ def element_order(self) -> tuple:
         Return a tuple value that is used in ordering elements
         of an expression. The tuple is ultimately compared lexicographically.
         """
-        return (
-            BASIC_ATOM_NUMBER_ELT_ORDER,
-            self.real.element_order[1],
-            self.imag.element_order[1],
-            1,
-        )
+        order_real, order_imag = self.real.element_order, self.imag.element_order
+
+        # If the real of the imag parts are real numbers, sort according
+        # the minimum precision.
+        # Example:
+        # Sort[{1+2I, 1.+2.I, 1.`4+2.`5I, 1.`2+2.`7 I}]
+        #
+        # = {1+2I, 1.+2.I, 1.`2+2.`7 I, 1.`4+2.`5I}
+        return order_real + order_imag
 
     @property
     def pattern_precedence(self) -> tuple:
@@ -965,9 +1001,13 @@ def user_hash(self, update) -> None:
 
     def __eq__(self, other) -> bool:
         if isinstance(other, Complex):
-            return self.real == other.real and self.imag == other.imag
-        else:
-            return super().__eq__(other)
+            return self.real.__eq__(other.real) and self.imag.__eq__(other.imag)
+        if isinstance(other, Number):
+            if abs(self.imag._value) != 0:
+                return False
+            return self.real.__eq__(other)
+
+        return super().__eq__(other)
 
     @property
     def is_zero(self) -> bool:
@@ -1019,6 +1059,17 @@ def __new__(cls, numerator, denominator=1) -> "Rational":
             self.hash = hash(key)
         return self
 
+    def __eq__(self, other) -> bool:
+        if isinstance(other, Rational):
+            return self.value.as_numer_denom() == other.value.as_numer_denom()
+        if isinstance(other, Integer):
+            return (other._value, 1) == self.value.as_numer_denom()
+        if isinstance(other, Number):
+            # For general numbers, rely on Real or Complex implementations.
+            return other.__eq__(self)
+        # General expressions
+        return super().__eq__(other)
+
     def __getnewargs__(self) -> tuple:
         return (self.numerator().value, self.denominator().value)
 
@@ -1078,7 +1129,7 @@ def element_order(self) -> tuple:
         return (
             BASIC_ATOM_NUMBER_ELT_ORDER,
             sympy.Float(self.value),
-            0,
+            1,
             1,
         )
 

test/builtin/test_file_operations.py

@@ -2,7 +2,7 @@
 """
 Unit tests for mathics.builtin.file_operations
 """
-
+import os
 import sys
 import time
 from test.helper import check_evaluation, evaluate
@@ -101,6 +101,10 @@
         ),
     ],
 )
+@pytest.mark.skipif(
+    os.getenv("SANDBOX", False),
+    reason="Test doesn't work in a sandboxed environment with access to local files",
+)
 def test_private_doctests_file_properties(str_expr, msgs, str_expected, fail_msg):
     """file_opertions.file_properties"""
     check_evaluation(

test/core/convert/test_mpmath.py

@@ -44,7 +44,8 @@ def test_from_to_mpmath():
         (MachineReal(1.2), MachineReal(1.2)),
         (PrecisionReal(SympyFloat(1.3, 10)), PrecisionReal(SympyFloat(1.3, 10))),
         (PrecisionReal(SympyFloat(1.3, 30)), PrecisionReal(SympyFloat(1.3, 30))),
-        (Complex(Integer1, IntegerM1), Complex(Integer1, IntegerM1)),
+        # After conversion, val1 == val2 but not SameQ[val1,val2]
+        # (Complex(Integer1, IntegerM1), Complex(Integer1, IntegerM1)),
         (Complex(Integer1, Real(-1.0)), Complex(Integer1, Real(-1.0))),
         (Complex(Real(1.0), Real(-1.0)), Complex(Real(1.0), Real(-1.0))),
         (

test/core/test_keycomparable.py

@@ -1,6 +1,161 @@
 import pytest
+from sympy import Float
 
-from mathics.core.atoms import Complex, Integer0, Integer1, Real, String
+from mathics.core.atoms import (
+    Complex,
+    Integer0,
+    Integer1,
+    PrecisionReal,
+    Rational,
+    Real,
+    String,
+)
+
+print("creating representations")
+ZERO_REPRESENTATIONS = {
+    "Integer": Integer0,
+    "MachineReal": Real(0.0),
+    "PrecisionReal`2": PrecisionReal(Float(0, 2)),
+    "PrecisionReal`5": PrecisionReal(Float(0, 5)),
+    "PrecisionReal`10": PrecisionReal(Float(0, 10)),
+    "PrecisionReal`20": PrecisionReal(Float(0, 20)),
+    "PrecisionReal`22": PrecisionReal(Float(0, 22)),
+    "PrecisionReal`40": PrecisionReal(Float(0, 40)),
+}
+ZERO_REPRESENTATIONS["Complex"] = Complex(
+    ZERO_REPRESENTATIONS["MachineReal"], ZERO_REPRESENTATIONS["MachineReal"]
+)
+ZERO_REPRESENTATIONS["Complex`20"] = Complex(
+    ZERO_REPRESENTATIONS["PrecisionReal`20"], ZERO_REPRESENTATIONS["PrecisionReal`20"]
+)
+
+ONE_REPRESENTATIONS = {
+    "Integer": Integer1,
+    "MachineReal": Real(1.0),
+    "PrecisionReal`2": PrecisionReal(Float(1, 2)),
+    "PrecisionReal`5": PrecisionReal(Float(1, 5)),
+    "PrecisionReal`10": PrecisionReal(Float(1, 10)),
+    "PrecisionReal`20": PrecisionReal(Float(1, 20)),
+    "PrecisionReal`22": PrecisionReal(Float(1, 22)),
+}
+
+
+# Add some complex cases
+ONE_REPRESENTATIONS["Complex Integer"] = Complex(
+    Integer1, ZERO_REPRESENTATIONS["PrecisionReal`10"]
+)
+ONE_REPRESENTATIONS["Complex"] = Complex(
+    ONE_REPRESENTATIONS["MachineReal"], ZERO_REPRESENTATIONS["MachineReal"]
+)
+ONE_REPRESENTATIONS["Complex`5"] = Complex(
+    ONE_REPRESENTATIONS["PrecisionReal`5"], ZERO_REPRESENTATIONS["PrecisionReal`5"]
+)
+
+
+ONE_FIFTH_REPRESENTATIONS = {
+    "Rational": Rational(1, 5),
+    "MachineReal": Real(0.2),
+    "PrecisionReal`20": PrecisionReal(Float(".2", 20)),
+    "PrecisionReal`22": PrecisionReal(Float(".2", 22)),
+}
+ONE_FIFTH_REPRESENTATIONS["Complex"] = Complex(
+    ONE_FIFTH_REPRESENTATIONS["MachineReal"], ZERO_REPRESENTATIONS["MachineReal"]
+)
+ONE_FIFTH_REPRESENTATIONS["Complex`20"] = Complex(
+    ONE_FIFTH_REPRESENTATIONS["PrecisionReal`20"],
+    ZERO_REPRESENTATIONS["PrecisionReal`20"],
+)
+
+
+def test_sorting_numbers():
+    """
+    In WMA, canonical order for numbers with the same value in different representations:
+    * Integer
+    * Complex[Integer, PrecisionReal]
+    * MachineReal
+    * Complex[MachineReal, MachineReal]
+    * PrecisionReal, Complex[PrecisionReal, PrecisionReal] if precision of the real parts are equal,
+    * otherwise, sort by precision of the real part.
+    * Rational
+    Example: {1, 1 + 0``10.*I, 1., 1. + 0.*I, 1.`4., 1.`4. + 0``4.*I, 1.`4. + 0``3.*I, 1.`6.}
+    and
+             {0.2, 0.2 + 0.*I, 0.2`4., 0.2`10., 1/5}
+    are lists in canonical order.
+
+    If the numbers are in different representations, numbers are sorted by their real parts,
+    and then the imaginary part is considered:
+    {0.2, 0.2 - 1.*I, 0.2 + 1.*I, 1/5}
+    """
+    zero_canonical_order = (
+        "Integer",
+        "MachineReal",
+        "Complex",
+        "PrecisionReal`20",
+        "Complex`20",
+        "PrecisionReal`22",
+    )
+    one_canonical_order = (
+        "Integer",
+        "MachineReal",
+        "Complex",
+        "Complex Integer",
+        "PrecisionReal`2",
+        "PrecisionReal`5",
+        "Complex`5",
+        "PrecisionReal`20",
+    )
+    one_fifth_canonical_order = (
+        "MachineReal",
+        "Complex",
+        "PrecisionReal`20",
+        "Complex`20",
+        "PrecisionReal`22",
+        "Rational",
+    )
+
+    # Canonical order
+    for order_equiv_forms in [
+        [ZERO_REPRESENTATIONS[pos] for pos in zero_canonical_order],
+        [ONE_REPRESENTATIONS[pos] for pos in one_canonical_order],
+        [ONE_FIFTH_REPRESENTATIONS[pos] for pos in one_fifth_canonical_order],
+    ]:
+        for elem, nelem in zip(order_equiv_forms[:-1], order_equiv_forms[1:]):
+            e_order, ne_order = elem.element_order, nelem.element_order
+            print("-------")
+            print(type(elem), f"{elem}", e_order)
+            print("vs", type(nelem), f"{nelem}", ne_order)
+            assert e_order < ne_order and not (
+                ne_order <= e_order
+            ), "wrong order or undefined."
+            assert (
+                elem == nelem
+            ), f"elements are not equal {elem} ({type(elem)}[{e_order}]) != {nelem}({type(nelem)}[{ne_order}])"
+            assert (
+                nelem == elem
+            ), f"elements are not equal {elem} ({type(elem)}[{e_order}]) != {nelem}({type(nelem)}[{ne_order}])"
+
+
+def test_sorting_complex():
+    one_fifth_rational = ONE_FIFTH_REPRESENTATIONS["Rational"]
+    one_fifth_mr = ONE_FIFTH_REPRESENTATIONS["MachineReal"]
+    one_fifth_pr = ONE_FIFTH_REPRESENTATIONS["PrecisionReal`20"]
+    one_fifth_cplx_i = Complex(one_fifth_mr, ONE_REPRESENTATIONS["MachineReal"])
+    one_fifth_cplx_mi = Complex(one_fifth_mr, -ONE_REPRESENTATIONS["MachineReal"])
+    canonical_sorted = [
+        one_fifth_mr,
+        one_fifth_cplx_mi,
+        one_fifth_cplx_i,
+        one_fifth_pr,
+        one_fifth_rational,
+    ]
+    for elem, nelem in zip(canonical_sorted[:-1], canonical_sorted[1:]):
+        e_order, ne_order = elem.element_order, nelem.element_order
+        print("-------")
+        print(type(elem), f"{elem}", e_order)
+        print("vs", type(nelem), f"{nelem}", ne_order)
+        assert e_order < ne_order and not (
+            ne_order <= e_order
+        ), f"{e_order}, {ne_order}"
 
 
 # Tests