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: sage-8.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 chapoton

  • Branch set to u/chapoton/22257
  • Commit set to 6a919fa0cb113b648fd79001cd4ef1afbf30a40b

New commits:

6a919fapy3 : removal of cmp() in real_lazy

comment:2 Changed 3 years ago by chapoton

  • Status changed from new to needs_review

waiting for the bots

comment:3 Changed 3 years ago by chapoton

  • Status changed from needs_review to needs_work

comment:4 Changed 3 years ago by chapoton

ooch, this one is going to be more complicated than expected..

comment:5 Changed 3 years ago by chapoton

  • 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:

ec2ca43trac 22257 get rid of cmp(), second tentative

comment:6 Changed 3 years ago by chapoton

Oooch, again. This is going to be *really* more complicated than expected.

comment:7 Changed 3 years ago by chapoton

  • Status changed from needs_review to needs_work

comment:8 Changed 3 years ago by chapoton

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 chapoton

  • Branch changed from u/chapoton/22257v2 to u/chapoton/22257v3
  • Commit changed from ec2ca43788b988b3234a90ed64100540d9254c49 to efa77ed34e7c6dd33701b2d378db556e7fa27b82
  • Status changed from needs_work to needs_review

another try, let us wait for the patchbots


New commits:

6a919fapy3 : removal of cmp() in real_lazy
9eb46b5Merge branch 'u/chapoton/22257' in 7.6.b6
efa77edtrac 22257 fixing doctests

comment:10 follow-up: Changed 3 years ago by chapoton

ok, this almost works, apparently.

Could some number-theorist please tell me if the failing doctest is really a failure ?

Last edited 3 years ago by chapoton (previous) (diff)

comment:11 Changed 3 years ago by jdemeyer

Would this work instead:

return richcmp(left.endpoints(), right.endpoints(), op)

comment:12 in reply to: ↑ 10 Changed 3 years ago by cremona

Replying to chapoton:

ok, this almost works, apparently.

Could some number-theorist 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 chapoton

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.

Last edited 3 years ago by chapoton (previous) (diff)

comment:14 Changed 3 years ago by chapoton

But wait, it seems that this number theoretical doctest failure disappears with the latest change suggested by Jeroen.

Last edited 3 years ago by chapoton (previous) (diff)

comment:15 Changed 3 years ago by chapoton

  • Branch changed from u/chapoton/22257v3 to u/chapoton/22257v4
  • Commit changed from efa77ed34e7c6dd33701b2d378db556e7fa27b82 to e1f07ca74416008e8280700152ec90e6e6f6329e

let me launch my patchbot on this new version


New commits:

e1f07catrac 22257 fixing back doctests

comment:16 Changed 3 years ago by cremona

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 chapoton

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 cremona

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 jdemeyer

Why do you need to add those bool() calls to various doctests?

comment:20 Changed 3 years ago by jdemeyer

  • 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 jdemeyer

Why this convoluted code

+        sage: x, y = xyz.xy()
+        sage: max(abs(x - 6), abs(y + 15)) < 1e-14
+        True

instead of abs_tol?

comment:22 Changed 3 years ago by git

  • Commit changed from e1f07ca74416008e8280700152ec90e6e6f6329e to a93ded97cb0210c094a361f671b315f9c8cc7615

Branch pushed to git repo; I updated commit sha1. New commits:

a93ded9trac 22257 details

comment:23 Changed 3 years ago by chapoton

  • 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 chapoton

green bot, please review

comment:25 Changed 3 years ago by chapoton

ping ?

comment:26 Changed 3 years ago by saraedum

  • 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 chapoton

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 jdemeyer

Wouldn't it be better to add the bool in _richcmp_?

comment:29 follow-up: Changed 3 years ago by chapoton

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 jdemeyer

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 follow-up: Changed 3 years ago by saraedum

  • 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 jdemeyer

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 vbraun

  • 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 vbraun

  • Commit a93ded97cb0210c094a361f671b315f9c8cc7615 deleted
  • Resolution fixed deleted
  • Status changed from closed to new

On OSX:

sage -t --long --warn-long 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/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 498, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/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/buildslave-sage/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/buildslave-sage/slave/sage_git/build/src/build/cythonized/sage/misc/classcall_metaclass.c:1410)
        return cls.classcall(cls, *args, **kwds)
      File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/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/buildslave-sage/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/buildslave-sage/slave/sage_git/build/src/build/cythonized/sage/misc/cachefunc.c:6077)
        w = self.f(*args, **kwds)
      File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/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/buildslave-sage/slave/sage_git/build/src/build/cythonized/sage/misc/classcall_metaclass.c:1859)
        return (<PyTypeObject*>type).tp_call(cls, args, kwds)
      File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/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 follow-up: Changed 3 years ago by chapoton

hell ! I have no apple on which to debug that..

comment:36 Changed 3 years ago by chapoton

  • Milestone changed from sage-7.6 to sage-8.0

comment:37 Changed 3 years ago by chapoton

  • Status changed from new to needs_review

comment:38 Changed 3 years ago by chapoton

  • Status changed from needs_review to needs_work

comment:39 Changed 3 years ago by chapoton

may be related to #16035 ?

comment:40 in reply to: ↑ 35 Changed 3 years ago by dimpase

Replying to chapoton:

hell ! I have no apple on which to debug that..

this is perfectly reproducible on FreeBSD 11.0 with clang 3.8, as described in #22679 FreeBSD 11 is some kind of approximation of OSX.

I took the current branch there and merged your u/chapoton/22257v4.

comment:41 Changed 3 years ago by chapoton

  • Branch changed from a93ded97cb0210c094a361f671b315f9c8cc7615 to u/chapoton/22257v4
  • Commit set to a93ded97cb0210c094a361f671b315f9c8cc7615

comment:42 Changed 3 years ago by chapoton

I made a branch u/chapoton/22257v5 where the richcmp are wrapped with bool

comment:43 Changed 3 years ago by chapoton

This really needs somebody with either osx or freebsd to do the debugging. Please help !

comment:44 Changed 3 years ago by chapoton

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 dimpase

here is FreeBSD with u/chapoton/22257_testing

sage -t --long --warn-long 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/site-packages/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/site-packages/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/site-packages/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/site-packages/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/site-packages/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 chapoton

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 dimpase

And, just in case, that's what I have at the prompt with this branch applied:

sage: bool(pi-pi == 0)
True
sage: bool(pi-pi < 0)
False
sage: bool(pi-pi > 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(pi-pi == 0)
True
sage: bool(pi-pi < 0)
False
sage: bool(pi-pi > 0)
False

(what happens is I suppose that pi-pi gets properly simplified to 0...)

comment:48 Changed 3 years ago by dimpase

sage: RealSet([pi, pi])
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-15-46f200bbe718> 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/site-packages/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/site-packages/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/site-packages/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 chapoton

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 follow-up: Changed 3 years ago by chapoton

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 dimpase

Replying to chapoton:

By the way, Dima, could you please check the status of #16035 and report there ?

just did; no problem in my setup.

comment:52 Changed 3 years ago by chapoton

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 dimpase

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 follow-up: Changed 3 years ago by 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

and

sage: bool(euler_gamma < euler_gamma)
False

comment:55 in reply to: ↑ 54 Changed 3 years ago by dimpase

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 chapoton

  • Branch changed from u/chapoton/22257v4 to u/chapoton/22257v6
  • Commit changed from a93ded97cb0210c094a361f671b315f9c8cc7615 to cec557e14b81b1e0b08c32eb77ac471ecbf9fb2b

Here is a new tentative. Dima, when you can, could you try with this branch (u/chapoton/22257v6), please ?


New commits:

00fc3f1Merge branch 'u/chapoton/22257v4' in 8.0.b0
cec557etrac 22257 trying to fix comparison pi < pi issue on osx
Last edited 3 years ago by chapoton (previous) (diff)

comment:57 Changed 3 years ago by chapoton

ok, at least the patchbot is still green on linux.

comment:58 Changed 3 years ago by dimpase

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
Last edited 3 years ago by dimpase (previous) (diff)

comment:59 Changed 3 years ago by chapoton

  • 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 chapoton

Green bot on linux and should be ok also on macosx.

The only change compared to the previously "positive-reviewed" 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 chapoton

ping ?

comment:62 Changed 3 years ago by tscrim

  • 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 vbraun

  • Branch changed from u/chapoton/22257v6 to cec557e14b81b1e0b08c32eb77ac471ecbf9fb2b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.