Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#22346 closed defect (fixed)

FormalSum should work with non-comparable values

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.6
Component: coercion Keywords: python3, richcmp
Cc: Merged in:
Authors: Jeroen Demeyer Reviewers: Marc Mezzarobba
Report Upstream: N/A Work issues:
Branch: bcc0a23 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

FormalSum sorts its entries, which is not guaranteed to work, especially on Python 3.

sage: from sage.structure.richcmp import *
sage: @richcmp_method
....: class NoCmp(object):
....:     def __richcmp__(self, other, op):
....:         if op not in (op_EQ, op_NE):
....:             raise RuntimeError
sage: nc = NoCmp()
sage: FormalSum([(1,nc), (1,1)])
RuntimeError                              Traceback (most recent call last)
<ipython-input-23-33e7842a248f> in <module>()
----> 1 FormalSum([(Integer(1),nc), (Integer(1),Integer(1))])

/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/structure/formal_sum.pyc in __init__(self, x, parent, check, reduce)
    132         assert isinstance(parent, parent.category().parent_class)
    133         if reduce:  # first reduce
--> 134             self.reduce()
    135         if check:   # then check
    136             k = parent.base_ring()

/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/structure/formal_sum.pyc in reduce(self)
    302             self._data = v
    303             return
--> 304         v.sort()
    305         w = []
    306         last = v[0][0]

/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/rings/integer.pyx in sage.rings.integer.Integer.__richcmp__ (build/cythonized/sage/rings/integer.c:7887)()
    961             c = mpz_cmp_d((<Integer>left).value, d)
    962         else:
--> 963             return coercion_model.richcmp(left, right, op)
    965         return rich_to_bool_sgn(op, c)

/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/structure/coerce.pyx in sage.structure.coerce.CoercionModel_cache_maps.richcmp (build/cythonized/sage/structure/coerce.c:20648)()
   1957         # we would end up trying the same coercion again.
   1958         if not y_is_Element and Py_TYPE(y).tp_richcompare:
-> 1959             res = Py_TYPE(y).tp_richcompare(y, x, revop(op))
   1960             if res is not NotImplemented:
   1961                 return res

/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/structure/richcmp.pyx in sage.structure.richcmp.slot_tp_richcompare (build/cythonized/sage/structure/richcmp.c:1768)()
    271     Function to put in the ``tp_richcompare`` slot.
    272     """
--> 273     return self.__richcmp__(other, op)

<ipython-input-19-9d5470810a1a> in __richcmp__(self, other, op)
      3     def __richcmp__(self, other, op):
      4         if op not in (op_EQ, op_NE):
----> 5             raise RuntimeError


To fix this, we do not sort the terms in reduce(). Instead, we keep the existing ordering.

This requires adding a few straightforward __hash__ functions in modular.

Change History (9)

comment:1 Changed 4 years ago by jdemeyer

  • Keywords richcmp added

comment:2 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 4 years ago by jdemeyer

  • Description modified (diff)
  • Milestone changed from sage-7.6 to sage-8.2

comment:4 Changed 4 years ago by chapoton

  • Keywords python3 added

comment:5 Changed 3 years ago by jdemeyer

  • Branch set to u/jdemeyer/formalsum_should_work_with_non_comparable_values

comment:6 Changed 3 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Commit set to bcc0a23bb4a32491f2145ff8412651a326b33a68
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

17a6ca1Add hash functions for ModularSymbol and ManinSymbol
9c92766Add comparisons and hash for CuspFamily
bcc0a23Do not sort terms in FormalSum

comment:7 Changed 3 years ago by mmezzarobba

  • Reviewers set to Marc Mezzarobba
  • Status changed from needs_review to positive_review

comment:8 Changed 3 years ago by vbraun

  • Branch changed from u/jdemeyer/formalsum_should_work_with_non_comparable_values to bcc0a23bb4a32491f2145ff8412651a326b33a68
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:9 Changed 3 years ago by embray

  • Commit bcc0a23bb4a32491f2145ff8412651a326b33a68 deleted
  • Milestone changed from sage-8.2 to sage-8.6
Note: See TracTickets for help on using tickets.