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:  sage7.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: 
Description (last modified by )
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
 Component changed from PLEASE CHANGE to symbolics
 Description modified (diff)
 Type changed from PLEASE CHANGE to defect
comment:2 Changed 7 years ago by
 Description modified (diff)
comment:3 Changed 7 years ago by
comment:4 Changed 7 years ago by
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
 Branch set to u/vbraun/symbolic_cmp
comment:6 Changed 7 years ago by
 Cc vdelecroix added
 Commit set to 62be17eb6eccffcddcc61e4f18a2123ec0c364d8
New commits:
62be17e  print_sorted and math_sorted utility functions

comment:7 Changed 7 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:8 Changed 7 years ago by
 Branch changed from u/vbraun/symbolic_cmp to u/rws/symbolic_cmp
comment:9 Changed 7 years ago by
 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:
215cff4  Merge branch 'develop' into t/16397/symbolic_cmp

af5f800  16397: use print_sorted() in two further places; fix doctest

comment:10 Changed 7 years ago by
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 returningFalse
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 sagedevel...
comment:11 Changed 7 years ago by
Yes, it's a bit over my head.
comment:12 followup: ↓ 13 Changed 6 years ago by
Was there ever any consensus on sagedevel on this? I'm leaning toward the first option, but not strongly.
comment:13 in reply to: ↑ 12 Changed 6 years ago by
Replying to kcrisman:
Was there ever any consensus on sagedevel on this? I'm leaning toward the first option, but not strongly.
You probably mean https://groups.google.com/forum/?hl=en#!searchin/sagedevel/cmp/sagedevel/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
 Milestone changed from sage6.4 to sage6.6
comment:15 Changed 6 years ago by
 Priority changed from major to critical
comment:16 Changed 6 years ago by
 Description modified (diff)
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?