Opened 11 years ago

Closed 11 years ago

#12353 closed defect (fixed)

wrong comparison between RealIntervalField and RealField

Reported by: Paul Zimmermann Owned by: Alex Ghitza
Priority: major Milestone: sage-5.0
Component: basic arithmetic Keywords:
Cc: Robert Bradshaw, William Stein Merged in: sage-5.0.beta4
Authors: David Roe Reviewers: Paul Zimmermann
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Paul Zimmermann)

on some machines we get the following:

sage: RealIntervalField(53)(-1) > RR(1)
False
sage: RealIntervalField(54)(-1) > RR(1)
True

The second answer True is clearly wrong.

If your machine gets the correct answer False in both cases, then it most probably gives:

sage: RealIntervalField(53)(1) > RR(-1)
True
sage: RealIntervalField(54)(1) > RR(-1)
False

where the second answer False is wrong.

Attachments (1)

12353.patch (10.5 KB) - added by Jeroen Demeyer 11 years ago.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 11 years ago by Paul Zimmermann

still wrong with Sage 5.0.beta1 on a 64-bit Core 2 under Ubuntu 10.10 (gcc 4.4.5):

tarte% ./sage
----------------------------------------------------------------------
| Sage Version 5.0.beta1, Release Date: 2012-01-22                   |
| Type notebook() for the GUI, and license() for information.        |
----------------------------------------------------------------------
**********************************************************************
*                                                                    *
* Warning: this is a prerelease version, and it may be unstable.     *
*                                                                    *
**********************************************************************
sage: RealIntervalField(53)(-1) > RR(1)
False
sage: RealIntervalField(54)(-1) > RR(1)
True

Paul

comment:2 Changed 11 years ago by Paul Zimmermann

Cc: Robert Bradshaw added

Robert, this seems related to the coercion model. Please could you help?

Paul

comment:3 Changed 11 years ago by Paul Zimmermann

I'm currently building Sage 4.8 on sage.math (where the bug doesn't happen with 4.7.2) to compare with my workstation where the bug happens, and isolate the problem.

Paul

comment:4 Changed 11 years ago by Paul Zimmermann

Cc: William Stein added
Description: modified (diff)

after some investigation, the function _richcmp in structure/element.pyx computes r = cmp(type(left), type(right)), which gives r=-1 on sage.math, and r=1 on my workstation (both with Sage 4.8).

Note that the *types* only are compared, thus by swapping the values I can produce a bug on sage.math too:

sage: RealIntervalField(54)(1) > RR(-1)
False

Something is badly wrong by comparing the types, not the values. Here we need help from someone more fluent in the Sage internals.

Paul

comment:5 Changed 11 years ago by François Bissey

Are we talking about this piece of code (line 840)

            except (TypeError, NotImplementedError):
                r = cmp(type(left), type(right))
                if r == 0:
                    r = -1

As far as I know comparing type when you cannot compare the values is standard python behavior and is implemented in other places such as sets/set.py

    def __cmp__(self, right):
        r"""
        Compare self and right.

        If right is not a Set compare types.  If right is also a Set,
        returns comparison on the underlying objects.

        .. note::

           If `X < Y` is true this does *not* necessarily mean
           that `X` is a subset of `Y`.  Also, any two sets can be
           compared still, but the result need not be meaningful
           if they are not equal. 

        EXAMPLES:
            sage: Set(ZZ) == Set(QQ)
            False
            sage: Set(ZZ) < Set(QQ)
            True
            sage: Primes() == Set(QQ)
            False

        The following is random, illustrating that comparison of
        sets is not the subset relation, when they are not equal::
        
            sage: Primes() < Set(QQ)             # random
            True or False
        """
        if not isinstance(right, Set_object):
            return cmp(type(self), type(right))
        return cmp(self.__object, right.__object)

The comparison of type can be machine dependent as far as I remember which explain the inconsistencies. I guess it would be nice to try to get

  • a better answer
  • a better comparison

comment:6 Changed 11 years ago by Paul Zimmermann

Francois,

Are we talking about this piece of code (line 840) [...]

yes exactly. But in this case the comparison of numerical values (the point interval [-1.0, -1.0] and the real 1.0) is well defined, thus I do not understand why the code returns a comparison of *types* which is machine dependent. And moreover I don't understand how the doctests could work before (see comment 50 in #12171).

Paul

comment:7 Changed 11 years ago by François Bissey

After thinking about it, I agree it should work. So something is wrong with the coercion (line 2774 onward) so we should try to debug what's happening there.

comment:8 Changed 11 years ago by Jeroen Demeyer

Priority: blockermajor

Why is this a blocker?

comment:9 in reply to:  8 Changed 11 years ago by Paul Zimmermann

Replying to jdemeyer:

Why is this a blocker?

because this is a wrong result in basic arithmetic, which can yield wrong results in any part of Sage.

William, Robert, are you out there to help fixing this?

Paul

comment:10 Changed 11 years ago by David Roe

I will take a look in the next few hours.

comment:11 Changed 11 years ago by David Roe

Alright. I know what's going on and am working on a fix. I'll try to post a patch tonight.

comment:12 Changed 11 years ago by David Roe

I didn't get to it tonight but will do so in the next couple days. Here's a brief description of the fix.

We need to update the construction functors returned by RIF.construction() and RR.construction() to be able to merge construction functors of different types. So there's a total ordering on the types of real fields, and the pushout of two real fields has type the "minimum" of their types, and precision the minimum of their precisions.

comment:13 Changed 11 years ago by David Roe

Status: newneeds_review

comment:14 Changed 11 years ago by Paul Zimmermann

Status: needs_reviewneeds_work
Work issues: remove RQDF reference

David, thanks for your patch, I will review it. A first comment: RQDF is deprecated, thus it should not be considered in the patch. Please can you update this?

Paul

comment:15 Changed 11 years ago by Paul Zimmermann

There is a new ticket #12458 about obsolete RQDF stuff.

Paul

comment:16 Changed 11 years ago by Paul Zimmermann

Description: modified (diff)

oops there was a typo in the description.

Paul

comment:17 Changed 11 years ago by David Roe

Status: needs_workneeds_review

How about the following? I made it clear that it was deprecated and removed it from pushout, but left it as an option in create_RealField where it returns RealField(212)? Let me know if you prefer a different solution.

comment:18 Changed 11 years ago by Paul Zimmermann

David,

since RQDF is since long time obsolete in Sage, I would prefer we don't add new references to it (see #12458).

Paul

comment:19 Changed 11 years ago by David Roe

Changed.

comment:20 Changed 11 years ago by Paul Zimmermann

Status: needs_reviewneeds_work
Work issues: remove RQDF referencefix typo

a typo while reading the patch: approrpiate should be appropriate.

Paul

comment:21 Changed 11 years ago by Paul Zimmermann

also, in We check that #12353 has been resolved, the 3rd and 4th tests are identical (my fault). The 3rd one should be at precision 53:

sage: RealIntervalField(53)(1) > RR(-1) 
True

Paul

comment:22 Changed 11 years ago by David Roe

Status: needs_workneeds_review

Fixed

comment:23 Changed 11 years ago by Paul Zimmermann

Authors: David Roe
Reviewers: Paul Zimmermann
Status: needs_reviewpositive_review
Work issues: fix typo

I give a positive review since all doctests still work, and the problem is fixed. However I'm not fluent enough in the construction functors to really review the new code. If Robert or William can do this, please go ahead. In the meantime, since this fixes an major defect, I set to positive review.

Paul

Changed 11 years ago by Jeroen Demeyer

Attachment: 12353.patch added

comment:24 Changed 11 years ago by Jeroen Demeyer

Folded both patches into one.

comment:25 Changed 11 years ago by Jeroen Demeyer

Merged in: sage-5.0.beta4
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.