Opened 6 years ago
Closed 6 years ago
#12353 closed defect (fixed)
wrong comparison between RealIntervalField and RealField
Reported by: | zimmerma | Owned by: | AlexGhitza |
---|---|---|---|
Priority: | major | Milestone: | sage-5.0 |
Component: | basic arithmetic | Keywords: | |
Cc: | robertwb, was | Merged in: | sage-5.0.beta4 |
Authors: | David Roe | Reviewers: | Paul Zimmermann |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
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)
Change History (26)
comment:1 Changed 6 years ago by
comment:2 Changed 6 years ago by
- Cc robertwb added
Robert, this seems related to the coercion model. Please could you help?
Paul
comment:3 Changed 6 years ago by
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 6 years ago by
- Cc was 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 6 years ago by
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 6 years ago by
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 6 years ago by
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 follow-up: ↓ 9 Changed 6 years ago by
- Priority changed from blocker to major
Why is this a blocker?
comment:9 in reply to: ↑ 8 Changed 6 years ago by
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 6 years ago by
I will take a look in the next few hours.
comment:11 Changed 6 years ago by
Alright. I know what's going on and am working on a fix. I'll try to post a patch tonight.
comment:12 Changed 6 years ago by
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 6 years ago by
- Status changed from new to needs_review
comment:14 Changed 6 years ago by
- Status changed from needs_review to needs_work
- Work issues set to 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 6 years ago by
There is a new ticket #12458 about obsolete RQDF stuff.
Paul
comment:16 Changed 6 years ago by
- Description modified (diff)
oops there was a typo in the description.
Paul
comment:17 Changed 6 years ago by
- Status changed from needs_work to needs_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 6 years ago by
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 6 years ago by
Changed.
comment:20 Changed 6 years ago by
- Status changed from needs_review to needs_work
- Work issues changed from remove RQDF reference to fix typo
a typo while reading the patch: approrpiate should be appropriate.
Paul
comment:21 Changed 6 years ago by
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:23 Changed 6 years ago by
- Reviewers set to Paul Zimmermann
- Status changed from needs_review to positive_review
- Work issues fix typo deleted
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 6 years ago by
comment:24 Changed 6 years ago by
Folded both patches into one.
comment:25 Changed 6 years ago by
- Merged in set to sage-5.0.beta4
- Resolution set to fixed
- Status changed from positive_review to closed
still wrong with Sage 5.0.beta1 on a 64-bit Core 2 under Ubuntu 10.10 (gcc 4.4.5):
Paul