Opened 3 years ago
Closed 3 years ago
#22257 closed enhancement (fixed)
py3: no cmp() in real lazy
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage8.0 
Component:  python3  Keywords:  days85 
Cc:  tscrim, aapitzsch, jdemeyer, saraedum  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Julian Rüth, Dima Pasechnik, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  cec557e (Commits)  Commit:  cec557e14b81b1e0b08c32eb77ac471ecbf9fb2b 
Dependencies:  Stopgaps: 
Description
as another step tp py3
Change History (63)
comment:1 Changed 3 years ago by
 Branch set to u/chapoton/22257
 Commit set to 6a919fa0cb113b648fd79001cd4ef1afbf30a40b
comment:3 Changed 3 years ago by
 Status changed from needs_review to needs_work
comment:4 Changed 3 years ago by
ooch, this one is going to be more complicated than expected..
comment:5 Changed 3 years ago by
 Branch changed from u/chapoton/22257 to u/chapoton/22257v2
 Commit changed from 6a919fa0cb113b648fd79001cd4ef1afbf30a40b to ec2ca43788b988b3234a90ed64100540d9254c49
 Status changed from needs_work to needs_review
here is another try ; let us wait for the bots
New commits:
ec2ca43  trac 22257 get rid of cmp(), second tentative

comment:6 Changed 3 years ago by
Oooch, again. This is going to be *really* more complicated than expected.
comment:7 Changed 3 years ago by
 Status changed from needs_review to needs_work
comment:8 Changed 3 years ago by
This triggers that:
sage: z4 = CyclotomicField(4).gen() sage: CDF(z4) 0.0
I have not been able to understand why.
comment:9 Changed 3 years ago by
 Branch changed from u/chapoton/22257v2 to u/chapoton/22257v3
 Commit changed from ec2ca43788b988b3234a90ed64100540d9254c49 to efa77ed34e7c6dd33701b2d378db556e7fa27b82
 Status changed from needs_work to needs_review
comment:10 followup: ↓ 12 Changed 3 years ago by
ok, this almost works, apparently.
Could some numbertheorist please tell me if the failing doctest is really a failure ?
comment:11 Changed 3 years ago by
Would this work instead:
return richcmp(left.endpoints(), right.endpoints(), op)
comment:12 in reply to: ↑ 10 Changed 3 years ago by
Replying to chapoton:
ok, this almost works, apparently.
Could some numbertheorist please tell me if the failing doctest is really a failure ?
Could you post here what the test is and what fails? To save time.
comment:13 Changed 3 years ago by
Yes, sure. Here is the existing doctest
sage: f = x^8  3*x^7 + 61/3*x^6  9*x^5 + 298*x^4 + 458*x^3 + 1875*x^2 + 4293*x + 3099 sage: F1 = NumberField(f, 'z', embedding=1.18126721294295 + 3.02858651117832j) sage: F2 = NumberField(f, 'z', embedding=1.18126721294295  3.02858651117832j) sage: (F, map1, map2, k) = F1.composite_fields(F2, both_maps=True)[0] sage: F.degree() 32 sage: F.gen() == map2(F2.gen()) + k*map1(F1.gen()) True
It fails by answering instead that F has degree 8 (just as F1, F2), with 2 identity maps as morphisms.
comment:14 Changed 3 years ago by
But wait, it seems that this number theoretical doctest failure disappears with the latest change suggested by Jeroen.
comment:15 Changed 3 years ago by
 Branch changed from u/chapoton/22257v3 to u/chapoton/22257v4
 Commit changed from efa77ed34e7c6dd33701b2d378db556e7fa27b82 to e1f07ca74416008e8280700152ec90e6e6f6329e
comment:16 Changed 3 years ago by
Degree 4 is certainly wrong since F1 contains only one root of f. Over F1, f factors into degrees 1, 3, 4. If the conjugate root is a root of the degree 4 factor then the degree of the composite would indeed be 8*4=32. I did not check whether the conjugate is a root of the degree 3 factor; if it was, the composite would have degree 24.
comment:17 Changed 3 years ago by
Thank you, John and Jeroen.
Bot is now green, so please review. Turns out to be not so complicated, in the end.
comment:18 Changed 3 years ago by
Just to confirm, I checked that after adjoining the first root, the second is a root of the degree 4 factor, so the compositum does have degree 8*4=32.
comment:19 Changed 3 years ago by
Why do you need to add those bool()
calls to various doctests?
comment:20 Changed 3 years ago by
 Status changed from needs_review to needs_work
You don't need this:
from cpython.object cimport Py_EQ
comment:21 Changed 3 years ago by
Why this convoluted code
+ sage: x, y = xyz.xy()
+ sage: max(abs(x  6), abs(y + 15)) < 1e14
+ True
instead of abs_tol
?
comment:22 Changed 3 years ago by
 Commit changed from e1f07ca74416008e8280700152ec90e6e6f6329e to a93ded97cb0210c094a361f671b315f9c8cc7615
Branch pushed to git repo; I updated commit sha1. New commits:
a93ded9  trac 22257 details

comment:23 Changed 3 years ago by
 Status changed from needs_work to needs_review
done.
For the added bool, they are necessary because otherwise one gets a symbolic equality. This is probably caused by a difference of behavior between cmp and richcmp in the symbolic ring (that we will have to handle at some point, for sure).
comment:24 Changed 3 years ago by
green bot, please review
comment:25 Changed 3 years ago by
ping ?
comment:26 Changed 3 years ago by
 Keywords days85 added
 Reviewers set to Julian Rüth
 Status changed from needs_review to needs_info
This looks good. I don't really understand the bool()
though. If we keep it like this, then code that compares elements of RLF
is going to break, right?
comment:27 Changed 3 years ago by
no, because the bool will be called by the "if" or "while" or whatever uses it as a test. Only bare equalities will stay unevaluated.
comment:28 Changed 3 years ago by
Wouldn't it be better to add the bool
in _richcmp_
?
comment:29 followup: ↓ 30 Changed 3 years ago by
Well, this is only needed for lazy_numbers initialized from the Symbolic Ring.
I would prefer to keep the bool where it is. This is really just a very tiny shadow of the fact that the symbolic ring has two different uses for cmp and richcmp. We will need to get rid of one of these at some point, but this is not the place to do so.
comment:30 in reply to: ↑ 29 Changed 3 years ago by
Replying to chapoton:
Well, this is only needed for lazy_numbers initialized from the Symbolic Ring.
This is really just a very tiny shadow of the fact that the symbolic ring has two different uses for cmp and richcmp. We will need to get rid of one of these at some point, but this is not the place to do so.
I agree with all of the above. Still, I don't like the fact that you are breaking RLF(1) < RLF(sqrt(2))
.
comment:31 followup: ↓ 32 Changed 3 years ago by
 Status changed from needs_info to positive_review
Ok, I see now. I think this is fine then. jdemeyer, please set this back to needs_work if you think that something needs to be done about the comparisons.
comment:32 in reply to: ↑ 31 Changed 3 years ago by
Replying to saraedum:
Ok, I see now. I think this is fine then. jdemeyer, please set this back to needs_work if you think that something needs to be done about the comparisons.
Well, I do think that there is something to be fixed here. But I don't feel strongly enough about it to set the ticket back to needs_work.
comment:33 Changed 3 years ago by
 Branch changed from u/chapoton/22257v4 to a93ded97cb0210c094a361f671b315f9c8cc7615
 Resolution set to fixed
 Status changed from positive_review to closed
comment:34 Changed 3 years ago by
 Commit a93ded97cb0210c094a361f671b315f9c8cc7615 deleted
 Resolution fixed deleted
 Status changed from closed to new
On OSX:
sage t long warnlong 126.9 src/sage/symbolic/expression.pyx ********************************************************************** File "src/sage/symbolic/expression.pyx", line 3537, in sage.symbolic.expression.Expression._cmp_ Failed example: RealSet((0, pi),[pi, pi],(pi,4)) Exception raised: Traceback (most recent call last): File "/Users/buildslavesage/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 498, in _run self.compile_and_execute(example, compiler, test.globs) File "/Users/buildslavesage/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 861, in compile_and_execute exec(compiled, globs) File "<doctest sage.symbolic.expression.Expression._cmp_[40]>", line 1, in <module> RealSet((Integer(0), pi),[pi, pi],(pi,Integer(4))) File "sage/misc/lazy_import.pyx", line 389, in sage.misc.lazy_import.LazyImport.__call__ (/Users/buildslavesage/slave/sage_git/build/src/build/cythonized/sage/misc/lazy_import.c:4007) return self._get_object()(*args, **kwds) File "sage/misc/classcall_metaclass.pyx", line 330, in sage.misc.classcall_metaclass.ClasscallMetaclass.__call__ (/Users/buildslavesage/slave/sage_git/build/src/build/cythonized/sage/misc/classcall_metaclass.c:1410) return cls.classcall(cls, *args, **kwds) File "/Users/buildslavesage/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/sets/real_set.py", line 644, in __classcall__ intervals.append(InternalRealInterval(lower, True, upper, True)) File "sage/misc/classcall_metaclass.pyx", line 330, in sage.misc.classcall_metaclass.ClasscallMetaclass.__call__ (/Users/buildslavesage/slave/sage_git/build/src/build/cythonized/sage/misc/classcall_metaclass.c:1410) return cls.classcall(cls, *args, **kwds) File "sage/misc/cachefunc.pyx", line 1059, in sage.misc.cachefunc.CachedFunction.__call__ (/Users/buildslavesage/slave/sage_git/build/src/build/cythonized/sage/misc/cachefunc.c:6077) w = self.f(*args, **kwds) File "/Users/buildslavesage/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/structure/unique_representation.py", line 1022, in __classcall__ instance = typecall(cls, *args, **options) File "sage/misc/classcall_metaclass.pyx", line 497, in sage.misc.classcall_metaclass.typecall (/Users/buildslavesage/slave/sage_git/build/src/build/cythonized/sage/misc/classcall_metaclass.c:1859) return (<PyTypeObject*>type).tp_call(cls, args, kwds) File "/Users/buildslavesage/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/sets/real_set.py", line 123, in __init__ raise ValueError('lower/upper bounds are not sorted') ValueError: lower/upper bounds are not sorted **********************************************************************
comment:35 followup: ↓ 40 Changed 3 years ago by
hell ! I have no apple on which to debug that..
comment:36 Changed 3 years ago by
 Milestone changed from sage7.6 to sage8.0
comment:37 Changed 3 years ago by
 Status changed from new to needs_review
comment:38 Changed 3 years ago by
 Status changed from needs_review to needs_work
comment:39 Changed 3 years ago by
may be related to #16035 ?
comment:40 in reply to: ↑ 35 Changed 3 years ago by
comment:41 Changed 3 years ago by
 Branch changed from a93ded97cb0210c094a361f671b315f9c8cc7615 to u/chapoton/22257v4
 Commit set to a93ded97cb0210c094a361f671b315f9c8cc7615
comment:42 Changed 3 years ago by
I made a branch u/chapoton/22257v5 where the richcmp are wrapped with bool
comment:43 Changed 3 years ago by
This really needs somebody with either osx or freebsd to do the debugging. Please help !
comment:44 Changed 3 years ago by
Could please at least someone with either a Mac or FreeBSD give me the result of the failing doctest above with the branch
u/chapoton/22257_testing
applied ?
comment:45 Changed 3 years ago by
here is FreeBSD with u/chapoton/22257_testing
sage t long warnlong 41.0 src/sage/symbolic/expression.pyx ********************************************************************** File "src/sage/symbolic/expression.pyx", line 3537, in sage.symbolic.expression.Expression._cmp_ Failed example: RealSet((0, pi),[pi, pi],(pi,4)) Exception raised: Traceback (most recent call last): File "/usr/home/dima/Sage/sage/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 498, in _run self.compile_and_execute(example, compiler, test.globs) File "/usr/home/dima/Sage/sage/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 861, in compile_and_execute exec(compiled, globs) File "<doctest sage.symbolic.expression.Expression._cmp_[40]>", line 1, in <module> RealSet((Integer(0), pi),[pi, pi],(pi,Integer(4))) File "sage/misc/lazy_import.pyx", line 389, in sage.misc.lazy_import.LazyImport.__call__ (/home/dima/Sage/sage/src/build/cythonized/sage/misc/lazy_import.c:4016) return self._get_object()(*args, **kwds) File "sage/misc/classcall_metaclass.pyx", line 330, in sage.misc.classcall_metaclass.ClasscallMetaclass.__call__ (/home/dima/Sage/sage/src/build/cythonized/sage/misc/classcall_metaclass.c:1415) return cls.classcall(cls, *args, **kwds) File "/usr/home/dima/Sage/sage/local/lib/python2.7/sitepackages/sage/sets/real_set.py", line 645, in __classcall__ intervals.append(InternalRealInterval(lower, True, upper, True)) File "sage/misc/classcall_metaclass.pyx", line 330, in sage.misc.classcall_metaclass.ClasscallMetaclass.__call__ (/home/dima/Sage/sage/src/build/cythonized/sage/misc/classcall_metaclass.c:1415) return cls.classcall(cls, *args, **kwds) File "sage/misc/cachefunc.pyx", line 1059, in sage.misc.cachefunc.CachedFunction.__call__ (/home/dima/Sage/sage/src/build/cythonized/sage/misc/cachefunc.c:6086) w = self.f(*args, **kwds) File "/usr/home/dima/Sage/sage/local/lib/python2.7/sitepackages/sage/structure/unique_representation.py", line 1022, in __classcall__ instance = typecall(cls, *args, **options) File "sage/misc/classcall_metaclass.pyx", line 497, in sage.misc.classcall_metaclass.typecall (/home/dima/Sage/sage/src/build/cythonized/sage/misc/classcall_metaclass.c:1864) return (<PyTypeObject*>type).tp_call(cls, args, kwds) File "/usr/home/dima/Sage/sage/local/lib/python2.7/sitepackages/sage/sets/real_set.py", line 124, in __init__ raise ValueError(msg.format(lower, upper)) ValueError: lower/upper bounds (3.141592653589794?/3.141592653589794?) are not sorted ********************************************************************** 1 item had failures: 1 of 44 in sage.symbolic.expression.Expression._cmp_ [2673 tests, 1 failure, 25.49 s]
comment:46 Changed 3 years ago by
Thanks a lot, Dima ! So the problem is not coming from where Volker thought it could come..
Could you just try RealSet([pi, pi])
and pi == pi
please ?
comment:47 Changed 3 years ago by
And, just in case, that's what I have at the prompt with this branch applied:
sage: bool(pipi == 0) True sage: bool(pipi < 0) False sage: bool(pipi > 0) False sage: pi<pi pi < pi sage: bool(pi<pi) True sage: bool(pi==pi) True sage: bool(pi>pi) True sage: bool(pi<4) True sage: bool(pi>4) False
from the point of view of numerical analysis, this all makes sense.
One cannot reliably compare floating point numbers for equality, and pi
is just
a floating point number in disguise. So you compare two "equal" floating point numbers for equality, this won't fly...
But the following seems to make sense:
sage: bool(pipi == 0) True sage: bool(pipi < 0) False sage: bool(pipi > 0) False
(what happens is I suppose that pipi gets properly simplified to 0...)
comment:48 Changed 3 years ago by
sage: RealSet([pi, pi])  ValueError Traceback (most recent call last) <ipythoninput1546f200bbe718> in <module>() > 1 RealSet([pi, pi]) /usr/home/dima/Sage/sage/src/sage/misc/lazy_import.pyx in sage.misc.lazy_import.LazyImport.__call__ (/home/dima/Sage/sage/src/build/cythonized/sage/misc/lazy_import.c:4016)() 387 True 388 """ > 389 return self._get_object()(*args, **kwds) 390 391 def __repr__(self): /usr/home/dima/Sage/sage/src/sage/misc/classcall_metaclass.pyx in sage.misc.classcall_metaclass.ClasscallMetaclass.__call__ (/home/dima/Sage/sage/src/build/cythonized/sage/misc/classcall_metaclass.c:1415)() 328 """ 329 if cls.classcall is not None: > 330 return cls.classcall(cls, *args, **kwds) 331 else: 332 # Fast version of type.__call__(cls, *args, **kwds) /usr/home/dima/Sage/sage/local/lib/python2.7/sitepackages/sage/sets/real_set.pyc in __classcall__(cls, *args) 643 elif isinstance(arg, list): 644 lower, upper = RealSet._prep(*arg) > 645 intervals.append(InternalRealInterval(lower, True, upper, True)) 646 elif isinstance(arg, InternalRealInterval): 647 intervals.append(arg) /usr/home/dima/Sage/sage/src/sage/misc/classcall_metaclass.pyx in sage.misc.classcall_metaclass.ClasscallMetaclass.__call__ (/home/dima/Sage/sage/src/build/cythonized/sage/misc/classcall_metaclass.c:1415)() 328 """ 329 if cls.classcall is not None: > 330 return cls.classcall(cls, *args, **kwds) 331 else: 332 # Fast version of type.__call__(cls, *args, **kwds) /usr/home/dima/Sage/sage/src/sage/misc/cachefunc.pyx in sage.misc.cachefunc.CachedFunction.__call__ (/home/dima/Sage/sage/src/build/cythonized/sage/misc/cachefunc.c:6086)() 1057 return self.cache[k] 1058 except KeyError: > 1059 w = self.f(*args, **kwds) 1060 self.cache[k] = w 1061 return w /usr/home/dima/Sage/sage/local/lib/python2.7/sitepackages/sage/structure/unique_representation.pyc in __classcall__(cls, *args, **options) 1020 True 1021 """ > 1022 instance = typecall(cls, *args, **options) 1023 assert isinstance( instance, cls ) 1024 if instance.__class__.__reduce__ == CachedRepresentation.__reduce__: /usr/home/dima/Sage/sage/src/sage/misc/classcall_metaclass.pyx in sage.misc.classcall_metaclass.typecall (/home/dima/Sage/sage/src/build/cythonized/sage/misc/classcall_metaclass.c:1864)() 495 TypeError: Argument 'cls' has incorrect type (expected type, got classobj) 496 """ > 497 return (<PyTypeObject*>type).tp_call(cls, args, kwds) 498 499 # Class for timing:: /usr/home/dima/Sage/sage/local/lib/python2.7/sitepackages/sage/sets/real_set.pyc in __init__(self, lower, lower_closed, upper, upper_closed, check) 122 if not(lower is minus_infinity or upper is infinity) and lower > upper: 123 msg = 'lower/upper bounds ({}/{}) are not sorted' > 124 raise ValueError(msg.format(lower, upper)) 125 if (lower_closed and lower == minus_infinity): 126 raise ValueError('interval cannot be closed at oo') ValueError: lower/upper bounds (3.141592653589794?/3.141592653589794?) are not sorted
comment:49 Changed 3 years ago by
ok, so the problem is here:
sage: bool(pi>pi) True
which is not the normal behaviour. I get False
with the same branch.
One possible solution (not solving the real issue) would be to replace the test ̀lower > upper
by lower  upper > 0
.
comment:50 followup: ↓ 51 Changed 3 years ago by
By the way, Dima, could you please check the status of #16035 and report there ?
comment:51 in reply to: ↑ 50 Changed 3 years ago by
comment:52 Changed 3 years ago by
Thanks. Could you please tell me what happens with
bool(e < e)
and
bool(sqrt(2) < sqrt(2))
Just to be sure we really have located the problem..
comment:53 Changed 3 years ago by
sage: e<e e < e sage: bool(e<e) False sage: bool(e>e) False sage: bool(e==e) True sage: type(e) <type 'sage.symbolic.constants_c.E'> sage: type(pi) <type 'sage.symbolic.expression.Expression'> sage: type(sqrt(2)) <type 'sage.symbolic.expression.Expression'> sage: sqrt(2)==sqrt(2) sqrt(2) == sqrt(2) sage: sqrt(2)<sqrt(2) sqrt(2) < sqrt(2) sage: bool(sqrt(2)<sqrt(2)) False sage: bool(sqrt(2)==sqrt(2)) True sage: bool(sqrt(2)>sqrt(2)) False
I wonder why pi
is not of type sage.symbolic.constants_c.E
, while e
is.
The latter explains why these comparison tests work.
With pi
vs sqrt(2)
, apparently simplification works better with algebraic numbers than with transcendentals.
comment:54 followup: ↓ 55 Changed 3 years ago by
good. So the problem is specific to pi and other similar constants. Could you please try
sage: from sage.symbolic.constants import Pi sage: mypi = Pi() sage: mypi.__lt__(pi) pi > 3.141592653589793 sage: bool(_) False
and
sage: bool(euler_gamma < euler_gamma) False
comment:55 in reply to: ↑ 54 Changed 3 years ago by
Replying to chapoton:
good. So the problem is specific to pi and other similar constants. Could you please try
sage: from sage.symbolic.constants import Pi sage: mypi = Pi() sage: mypi.__lt__(pi) pi > 3.141592653589793 sage: bool(_) False
this is OK.
and
sage: bool(euler_gamma < euler_gamma) False
no, this is not
sage: bool(euler_gamma < euler_gamma) True sage: bool(euler_gamma > euler_gamma) True sage: bool(euler_gamma == euler_gamma) True sage: type(euler_gamma) <type 'sage.symbolic.expression.Expression'>
so euler_gamma
is as bad as pi
here.
comment:56 Changed 3 years ago by
 Branch changed from u/chapoton/22257v4 to u/chapoton/22257v6
 Commit changed from a93ded97cb0210c094a361f671b315f9c8cc7615 to cec557e14b81b1e0b08c32eb77ac471ecbf9fb2b
comment:57 Changed 3 years ago by
ok, at least the patchbot is still green on linux.
comment:58 Changed 3 years ago by
OK, u/chapoton/22257v6
passes the long tests in src/sage/symbolic/expression.pyx
.
And the following also looks sane:
sage: pi<pi pi < pi sage: bool(pi<pi) False sage: bool(pi>pi) False sage: bool(pi==pi) True
comment:59 Changed 3 years ago by
 Cc tscrim aapitzsch jdemeyer saraedum added
 Status changed from needs_work to needs_review
ok, then let me ask again for a review
comment:60 Changed 3 years ago by
Green bot on linux and should be ok also on macosx.
The only change compared to the previously "positivereviewed" branch is the removal of the method __lt__
in the class Constant
inside the file constants.py
. Some doctests have also been added in expressions.pyx.
This means that comparison with pi and similar constants will be handled (in a better way) by the general comparison of expressions.
Once again, this is currently the point where python3 compilation fails. And this is needed to go forward, with next step being 22297.
Please review!
comment:61 Changed 3 years ago by
ping ?
comment:62 Changed 3 years ago by
 Reviewers changed from Julian Rüth to Julian Rüth, Dima Pasechnik, Travis Scrimshaw
 Status changed from needs_review to positive_review
I'm good sending this back to the buildbots.
comment:63 Changed 3 years ago by
 Branch changed from u/chapoton/22257v6 to cec557e14b81b1e0b08c32eb77ac471ecbf9fb2b
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
py3 : removal of cmp() in real_lazy