Opened 7 years ago

Last modified 5 years ago

#16397 closed defect

Symbolic cmp — at Version 16

Reported by: vbraun Owned by:
Priority: critical Milestone: sage-7.1
Component: symbolics Keywords:
Cc: vdelecroix, mmezzarobba, jpflori Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: u/rws/symbolic_cmp (Commits, GitHub, GitLab) Commit: af5f800fdc616022d503b5e845ec81ebfef7c25e
Dependencies: Stopgaps:

Status badges

Description (last modified by rws)

In the symbolic ring, cmp implements the print comparison which is probably not what you envisioned:

sage: cmp(1, sqrt(2))     # mathematically correct, uses rich comparison
-1
sage: cmp(SR(1), sqrt(2)) # unexpectedly, you get the print sort order
1

Everybody who coerces to same parents internally before comparing trips over this, for example the real lazy field:

sage: RLF(1) < RLF(sqrt(2))
False

This also makes RealSet unusable with symbolics:

sage: RealSet((0, pi),[pi, pi],(pi,4))
[pi, 4)
sage: RealSet((0, pi),[0, pi],(pi,4))
[pi, 4)
sage: RealSet((0, pi),[0, 3.5],(pi,4))
(pi, 4)

Change History (16)

comment:1 Changed 7 years ago by vbraun

  • Component changed from PLEASE CHANGE to symbolics
  • Description modified (diff)
  • Type changed from PLEASE CHANGE to defect

comment:2 Changed 7 years ago by vbraun

  • Description modified (diff)

comment:3 Changed 7 years ago by burcin

I can see how that might be surprising. :)

IIRC, that was a conscious decision to expose the print order from Pynac to Python/Sage to be used for sorting lists, etc. where symbolic expressions occur. I guess many places in the code call cmp() instead of using the comparison operators.

We would need to get rid of this design for Python 3 compatibility anyway. Shall we remove support for cmp() and change all places where lists are returned from symbolics to explicitly call the print order?

comment:4 Changed 7 years ago by vbraun

Yes, thats what I'm thinking as well. I'll write a print_order function and see if I can get rid of all cmp calls...

comment:5 Changed 7 years ago by vbraun

  • Branch set to u/vbraun/symbolic_cmp

comment:6 Changed 7 years ago by vdelecroix

  • Cc vdelecroix added
  • Commit set to 62be17eb6eccffcddcc61e4f18a2123ec0c364d8

New commits:

62be17eprint_sorted and math_sorted utility functions

comment:7 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:8 Changed 7 years ago by rws

  • Branch changed from u/vbraun/symbolic_cmp to u/rws/symbolic_cmp

comment:9 Changed 7 years ago by rws

  • Commit changed from 62be17eb6eccffcddcc61e4f18a2123ec0c364d8 to af5f800fdc616022d503b5e845ec81ebfef7c25e

Trying to push this ticket a bit. I also found this cmp call:

 File "matrix_integer_dense.pyx", line 604, in sage.matrix.matrix_integer_dense.Matrix_integer_dense.__richcmp__ (build/cythonized/sage/matrix/matrix_integer_dense.c:9503)
  File "element.pyx", line 873, in sage.structure.element.Element._richcmp (build/cythonized/sage/structure/element.c:8891)
  File "element.pyx", line 855, in sage.structure.element.Element._richcmp_ (build/cythonized/sage/structure/element.c:8615)
  File "element.pyx", line 902, in sage.structure.element.Element._richcmp (build/cythonized/sage/structure/element.c:9316)
  File "element.pyx", line 949, in sage.structure.element.Element._richcmp_c_impl (build/cythonized/sage/structure/element.c:9645)
  File "matrix_dense.pyx", line 126, in sage.matrix.matrix_dense.Matrix_dense._cmp_c_impl (build/cythonized/sage/matrix/matrix_dense.c:3092)
  File "expression.pyx", line 3066, in sage.symbolic.expression.Expression.__cmp__ (build/cythonized/sage/symbolic/expression.cpp:16865)

Do I understand it right that this should call math_sorted instead of cmp?


New commits:

215cff4Merge branch 'develop' into t/16397/symbolic_cmp
af5f80016397: use print_sorted() in two further places; fix doctest

comment:10 Changed 7 years ago by vbraun

The problem is IMHO what to do with Python 3. Sure we can play the two different comparisons (cmp vs. rich) in Python 2 against each other, but in Py3 there will be only one comparison. So either

  • Comparison is always symbolic, and sorting will have to call __bool__ on the symbolic inequalities. We then need to make that a total order instead of returning False all the time. Slow.
  • Comparison is never symbolic, and you need to call x.symbolic_less(y). Could be beautified by the preparser. The actual comparison returns the internal term order. Fast.

Thought? Maybe that topic is more fodder for sage-devel...

Last edited 7 years ago by vbraun (previous) (diff)

comment:11 Changed 7 years ago by rws

Yes, it's a bit over my head.

comment:12 follow-up: Changed 6 years ago by kcrisman

Was there ever any consensus on sage-devel on this? I'm leaning toward the first option, but not strongly.

comment:13 in reply to: ↑ 12 Changed 6 years ago by rws

Replying to kcrisman:

Was there ever any consensus on sage-devel on this? I'm leaning toward the first option, but not strongly.

You probably mean https://groups.google.com/forum/?hl=en#!searchin/sage-devel/cmp/sage-devel/092yBmHfXQo/4qfS_JHLJdwJ and #16537, #17175. The newsgroup thread is mainly about equality/hashing and a bit of richcmp, not the two choices above. I am just starting to read about this.

comment:14 Changed 6 years ago by rws

  • Milestone changed from sage-6.4 to sage-6.6

comment:15 Changed 6 years ago by rws

  • Priority changed from major to critical

comment:16 Changed 6 years ago by rws

  • Description modified (diff)
Note: See TracTickets for help on using tickets.