bugs in comparisons between constants, wrapped pyobjects, infinity

[dkrenn]

We have

sage: bool(pi<Infinity)
False
sage: bool(pi>Infinity)
True

which is obviously wrong. It seems that the problem only occurs with pi, because the following give correct results

sage: bool(pi<2*pi)
True
sage: bool(2*pi<Infinity)
True
sage: bool(e<Infinity)  
True
sage: bool(e<pi)
True

This was reported on ​sage-support by Robert Samal.

See the discussion at ​https://groups.google.com/forum/?hl=en#!topic/sage-devel/Oip2hzvjFZQ

​https://github.com/pynac/pynac/issues/69

---

Previously proposed patch: trac_12967-symbolic_ring-review.patch​.

[burcin]

ATM, this is caused by #11506:

sage: pi.pyobject()
pi
sage: type(pi.pyobject())
<class 'sage.symbolic.constants.Pi'>
sage: pi.pyobject() < oo        
False
sage: pi.pyobject() > oo
True

With #12950, comparison of infinities in Pynac changed. Now I get:

sage: bool(pi>Infinity)
False
sage: bool(pi<Infinity) 
False

which is better. I hope with the ordering patches in the Pynac queue this will improve.

The results of comparison with e is not relevant in this case. e is not a constant in Pynac, it is the exp() function. Once you form the relational expression e < Infinity, the comparison is handled differently.

I suggest adding a patch with doctests reflecting the new behavior with #12950 and closing this ticket when #12950 is merged.

[tscrim]

Here's the quick patch with some doctests reflecting the new behavior.

[kcrisman]

----------------------------------------------------------------------
| Sage Version 5.4.1, Release Date: 2012-11-15                       |
| Type "notebook()" for the browser-based notebook interface.        |
| Type "help()" for help.                                            |
----------------------------------------------------------------------
sage: bool(pi < infinity)
False
sage: bool(oo < pi)
False

That is what should actually be tested, as Burcin points out. Also, I feel like this is unintuitive enough of behavior (that pi is more or less incomparable with infinity and not like this)

sage: bool(2< oo)
True

that we should say something to that effect here, maybe even elsewhere in comparison doc where we have other examples saying that > gives False if we can't prove it's True.

I'm also wondering whether this is really "fixed" and deserves that doctest status.

[tscrim]

How about this? Can you think of any other places to put the warning about symbolic ring comparisons? Thanks.

[kcrisman]

There is a typo I am fixing in a review patch.

Otherwise I'll give the patch qua patch positive review - I don't think there is more you can do here, this seems fine and passes tests etc. - but wait on a comment from Burcin or someone else as to whether this should count as a fix before pressing the button. I guess I'm just a little uncomfortable with that, though I understand the sentiment and could be easily persuaded by a third party.

[kcrisman]

Apply only this patch

[kcrisman]

Patchbot, apply trac_12967-symbolic_ring-review.patch

[tscrim]

I'll let someone else set this to positive review, and I do understand your discomfort. Thank you for the review.

[ppurka]

This is absurd. Why should bool(pi < oo) return False? If we have symbolic constants, then we should be able to coerce them to some ring, possibly RR, and do the comparison.

[pbruin]

Replying to ppurka:

This is absurd. Why should bool(pi < oo) return False? If we have symbolic constants, then we should be able to coerce them to some ring, possibly RR, and do the comparison.

I agree. This comparison is apparently done by Pynac. Using Maxima gives the correct result:

sage: e = (SR(pi) < SR(Infinity)); e
pi < +Infinity
sage: bool(e)
False
sage: maxima(e)
%pi<inf
sage: bool(maxima(e))
True

(This is not fixed by #11506, by the way.)

[vbraun]

See #16397 for more (but unrelated, I think) SR comparison issues.

[vbraun]

Also interesting:

sage: cmp(SR(oo), sqrt(2))
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)
<ipython-input-7-aaaac68122b2> in <module>()
----> 1 cmp(SR(oo), sqrt(Integer(2)))

/home/vbraun/Code/sage.git/local/lib/python2.7/site-packages/sage/symbolic/expression.so in sage.symbolic.expression.Expression.__cmp__ (sage/symbolic/expression.cpp:16780)()

/home/vbraun/Code/sage.git/local/lib/python2.7/site-packages/sage/structure/element.so in sage.structure.element.Element._cmp (sage/structure/element.c:8490)()

/home/vbraun/Code/sage.git/local/lib/python2.7/site-packages/sage/symbolic/expression.so in sage.symbolic.expression.Expression._cmp_c_impl (sage/symbolic/expression.cpp:16834)()

RuntimeError: comparing typeid's

Print order is broken for some combination of symbolic ex'es.

[rws]

Replying to burcin:

sage: bool(pi>Infinity)
False
sage: bool(pi<Infinity) 
False

which is better. I hope with the ordering patches in the Pynac queue this will improve.

Still not improved. What is meant exactly here? Does someone have a link to a ticket somewhere?

[jakobkroeker]

if something is incomparable or the answer is not known the way to go is an exception; see discussion at ​https://groups.google.com/forum/#!msg/sage-devel/vNxnHSeRBW4/0OpeL0yv9YUJ (or a multistate result, which cannot be converted or compared with a bool (True/False);

Everything else is in my opinion a major design flaw and will lead to plethora of bugs !!!

[rws]

This utilizes the InfinityRing for comparison. Doctest failures related to infinities show there is more to do.

Replying to jakobkroeker:

if something is incomparable or the answer is not known the way to go is an exception

You will understand this all cannot be part of this ticket. EDIT: Actually the failing doctests support your opinion, and probably I better adapt the tests to the new behaviour.

---

New commits:

​8ef555c

12967: comparisons between constants, wrapped pyobjects, infinity

[git]

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

​8861762

12967: put back some removed code; fixes doctests

[rws]

sage -t --long src/sage/structure/parent.pyx
**********************************************************************
File "src/sage/structure/parent.pyx", line 1224, in sage.structure.parent.Parent.__contains__
Failed example:
    pi in RIF # there is no element of RIF equal to pi
Expected:
    False
Got:
    True

[git]

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

​7ac556f

12967: interval comparison now raises TypeError

[git]

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

​91cd2a6

12967: improve previous fix

[rws]

It was wrong to always raise an exception when an interval was compared. The reason why pi in RIF was True rather lies in structure.parent.py:Parent.contains() where the line

EQ = (x2 == x)

didn't account for symbolics and should read

EQ = bool(x2 == x)

This uncovers another bug in Expression.nonzero() which shows as

sage: bool(sqrt(2)==CC(sqrt(2)))
False

because sqrt(2) == 1.41421356237309 is False. Any ideas how to fit this in the current code?

EDIT: See also ​https://groups.google.com/forum/?hl=en#!topic/sage-devel/QBAipylR6iM

[git]

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

​0adb1d3

12967: dedicated CC.__contains__

[rws]

Resolved by a dedicated CC.__contains__.

[rws]

No, I give up. Better fix pynac.

[rws]

Reboot. This branch has clean logic handling all relations that can be decided without Maxima. I have left in the call to Pynac's relational_to_bool although it could possibly replaced too. The branch depends on #17984.

However, there is one doctest failure:

File "src/sage/structure/parent.pyx", line 1224, in sage.structure.parent.Parent.__contains__
Failed example:
    pi in RIF # there is no element of RIF equal to pi
Expected:
    False
Got:
    True

It is caused because Parent.__contains__ assumes wrongly that all equations that are of type Expression must be true, and pi == RIF(pi) isn't. In develop this is masked by code catching all kinds of exceptions that hides the fact that the conversion of the relation to Maxima throws an exception. The code takes this to mean that pi == RIF(pi) is False. In contrast this branch determines without invoking Maxima correctly that the relation is False but the fact no exception is thrown leads to Parent.__contains__ returning True! Since it's clearly wrong here to throw an exception just to oblige Parent.__contains__ that method in turn must be fixed.

---

New commits:

​39ab5b2	17984: Dedicated RR.__contains__() and CC.__contains__()
​a22a6e4	17984: description in docstring; fixes and doctests
​544450e	17984: fix typo in doctest
​c5845f6	12967: get relations with infinities right

[pbruin]

I'm afraid this implementation of __contains__() will make Mod(1, 2) in RR return True, which is obviously wrong. This is probably the reason why the nonsense input in the Chebyshev doctests is no longer detected.

I am in general hesitant about the idea of needing a custom __contains__() method; the strategy to implement x in P as bool(P(x) == x) basically seems a good one to me. How about fixing __cmp__() (or similar) when comparing symbolic expressions with numerical approximations so that (for example) bool(pi == 3.14[...]) returns True if the approximation is correct to the given precision?

[mmezzarobba]

Naïve question: what would be the problem with simply making SR coerce into InfinityRing instead of the reverse?

[mmezzarobba]

Replying to pbruin:

bool(pi == 3.14[...]) returns True if the approximation is correct to the given precision?

This sounds very dangerous to me.

[rws]

Replying to pbruin:

...the strategy to implement x in P as bool(P(x) == x) basically seems a good one to me.

It would be if users wanted x in RR/CC to mean x element of real/complex field of exactly this precision but the overwhelming majority want it to mean x element of real/complex field regardless of precision. And that's also the use cases within Sage that I have seen, and so I reflected it in the method description.

[pbruin]

Replying to rws:

Replying to pbruin:

...the strategy to implement x in P as bool(P(x) == x) basically seems a good one to me.

It would be if users wanted x in RR/CC to mean x element of real/complex field of exactly this precision but the overwhelming majority want it to mean x element of real/complex field regardless of precision. And that's also the use cases within Sage that I have seen, and so I reflected it in the method description.

But currently the precision is already taken care of in this way:

sage: pi.n(10) in RR
True
sage: pi.n(100) in RealField(50)
True

Isn't this what you want to keep as the correct behaviour?

[mmezzarobba]

Replying to rws:

the overwhelming majority want it to mean x element of real/complex field regardless of precision. And that's also the use cases within Sage that I have seen, and so I reflected it in the method description.

I view that as a misunderstanding due to the unfortunate use of the name RR in sage for what actually is the set of 53-bit floating-point numbers (with some exponent bounds). In almost all other respects, the elements of RR do not behave like "the reals", and it would be wrong to view them as such!

[pbruin]

Replying to mmezzarobba:

Replying to pbruin:

bool(pi == 3.14[...]) returns True if the approximation is correct to the given precision?

This sounds very dangerous to me.

In fact, bool(pi == pi.n()) already returns True, which I think is reasonable. In any case it is consistent with how elements of real fields of different precisions are compared.

The question is whether we also want to use this for x in RR. I currently cannot think of a natural way of implementing this. Just checking whether x can be converted in to RR is not enough (as the example with Mod(1, 2) in comment:35 shows); one would at least expect that x can be converted into RR and that the result is "sufficiently equal" to the original x.

[rws]

Replying to mmezzarobba:

I view that as a misunderstanding

Wrong perspective. Rather a bad design decision.

[pbruin]

Replying to mmezzarobba:

Replying to rws:

the overwhelming majority want it to mean x element of real/complex field regardless of precision. And that's also the use cases within Sage that I have seen, and so I reflected it in the method description.

I view that as a misunderstanding due to the unfortunate use of the name RR in sage for what actually is the set of 53-bit floating-point numbers (with some exponent bounds). In almost all other respects, the elements of RR do not behave like "the reals", and it would be wrong to view them as such!

From that perspective, pi in RR should return False. I personally wouldn't be opposed to this, but since there is some subtlety involved, I would expect many users won't like this.

[pbruin]

Maybe we should discourage users from using x in RR. There is the following alternative:

sage: pi.is_real()
True

[rws]

Replying to pbruin:

I currently cannot think of a natural way of implementing this.

Why generalize? The method could check if item has a method is_in_RR() which implementation in a new set is the responsibility of its author.

[rws]

Replying to pbruin:

Maybe we should discourage users from using x in RR. There is the following alternative:

sage: pi.is_real()
True

I'm fine with that too. RR.__contains__() is then used for the error.

[ppurka]

Replying to pbruin:

Maybe we should discourage users from using x in RR. There is the following alternative:

sage: pi.is_real()
True

I don't think this is a good idea. This breaks consistency across the different fields. Sage is quite inconsistent already from a user point of view, we shouldn't introduce more such inconsistencies.

[pbruin]

Replying to rws:

Replying to pbruin:

I currently cannot think of a natural way of implementing this.

Why generalize? The method could check if item has a method is_in_RR() which implementation in a new set is the responsibility of its author.

I meant I cannot think of a better way than bool(RR(x) == x). What you propose sounds rather ad hoc; if we need a special method for RR, then we would also need special cases for other inexact rings like CC, p-adic rings and power series rings... At most we could use the is_real() method, but looking a bit more into this, I think it does not work very well either:

sage: e.is_real()
False

[pbruin]

Replying to ppurka:

Replying to pbruin:

Maybe we should discourage users from using x in RR. There is the following alternative:

sage: pi.is_real()
True

I don't think this is a good idea. This breaks consistency across the different fields.

Only if you identify RR with the mathematical field R of real numbers. If we are serious about presenting RR as a certain set of numbers that is quite different from the mathematical field R (for example, Infinity in RR already returns True), then we can just let pi in RR return False, emphatically warn users about this, and say that different methods are needed to decide if an expression represents a mathematical real number.

[mmezzarobba]

Replying to pbruin:

Only if you identify RR with the mathematical field R of real numbers. If we are serious about presenting RR as a certain set of numbers that is quite different from the mathematical field R (for example, Infinity in RR already returns True), then we can just let pi in RR return False, emphatically warn users about this, and say that different methods are needed to decide if an expression represents a mathematical real number.

Exactly. If we want consistency across Sage, then the semantics of x in RR *cannot* be "x is a real number"; it must be "x is an element of the parent RR" or something similar.

[rws]

Replying to pbruin:

... What you propose sounds rather ad hoc; if we need a special method for RR, then we would also need special cases for other inexact rings like CC, p-adic rings and power series rings...

Bug fixing is always ad hoc. You can only make it easier (but by a huge amount) with good design and a well thought out model. Since no good model presents itself and delegating the task to the item is good design this would be an ansatz.

[jdemeyer]

Replying to rws:

Replying to pbruin:

...the strategy to implement x in P as bool(P(x) == x) basically seems a good one to me.

It would be if users wanted x in RR/CC to mean x element of real/complex field of exactly this precision.

I don't get why you say this. Doesn't bool(P(x) == x) work regardless of precision?

[pbruin]

Replying to rws:

Replying to pbruin:

... What you propose sounds rather ad hoc; if we need a special method for RR, then we would also need special cases for other inexact rings like CC, p-adic rings and power series rings...

Bug fixing is always ad hoc.

Not at all; I often find that a good first step towards fundamentally fixing a bug is trying to remove hacks that someone else put in to make a specific example work.

You can only make it easier (but by a huge amount) with good design and a well thought out model. Since no good model presents itself and delegating the task to the item is good design this would be an ansatz.

I disagree that no good model presents itself; defining x in P as bool(P(x) == x) seems like a good programming model to me, since it is simple and predictable.

The more I think about it, the more the fundamental problem does not seem to be membership testing, but comparison between real numbers defined in different approximations of the real numbers. Do we want RR(sqrt(2)) to be equal to SR(sqrt(2)) or not? And what about RR(1/3) and QQ(1/3)?

[rws]

Replying to jdemeyer:

I don't get why you say this. Doesn't bool(P(x) == x) work regardless of precision?

There seemed to be a precision problem: ​https://groups.google.com/d/topic/sage-devel/QBAipylR6iM/discussion

[mmezzarobba]

Replying to pbruin:

Not at all; I often find that a good first step towards fundamentally fixing a bug is trying to remove hacks that someone else put in to make a specific example work.

I strongly agree with that. Making specific examples work with no regard for global consistency usually means introducing bugs, not fixing them.

You can only make it easier (but by a huge amount) with good design and a well thought out model. Since no good model presents itself and delegating the task to the item is good design this would be an ansatz.

I disagree that no good model presents itself; defining x in P as bool(P(x) == x) seems like a good programming model to me, since it is simple and predictable.

I tend to agree. Note however that the current definition is more along the lines of parent(x) == P or P(x) == x, which may not be the same (think of intervals or objects that may not compare as equal to copies of themselves).

The more I think about it, the more the fundamental problem does not seem to be membership testing, but comparison between real numbers defined in different approximations of the real numbers. Do we want RR(sqrt(2)) to be equal to SR(sqrt(2)) or not? And what about RR(1/3) and QQ(1/3)?

As I repeat on every possible occasion, comparisons in sage are broken in more ways than I can count. One of the fundamental reasons IMHO is that there should be at least two kinds of comparisons (in addition to is), call them strict comparisons and semantic comparisons. Under strict equality, for instance, elements of different parents would always compare as unequal, and elements of a parent with no normal form would typically compare as equal only if they have the same syntactic representation. Semantic equality however could attempt coercing the elements in a common parent much like == currently does. (I believe that == should refer to the strict equality since, among other things, that would avoid many of the problems with hashing. But I know that many people disagree.)

Clearly we are talking about semantic equality here, but I believe making the previous distinction (at least conceptually) helps separating the issues. Now about your examples: with the current model with a single equality predicate, my answer is NOOO!!! in both cases. With a separate strict equality, it is less clear-cut, but intuitively I would still expect both results to be False.

[rws]

Given that it would be more consistent (but less user-friendly) to not mix R with RR, and item.is_real() is buggy, the fix of that is critical (#16436, #17117 are aspects). The Parent.__contains__ method would have to be adapted and a warning added. Maybe this even leads to a more consistent Parent.__contains__. The fix of the behaviour of bool(item==P(item)) is then less urgent (because it's rarely used otherwise). As to this ticket where failing comparisons with oo are described, the code does fix these but leaves at least the pi in RIF doctest failing (explanation in comment:33), and possibly more if I remove the RR/CC.__contains__. This could benefit from a more consistent Parent.__contains__ so let's go on.

[vbraun]

The symbolic constants, like pi.pyobject(), should be elements in some parent set. The symbolic constants can then coerce into the infinity ring, solving the pi.pyobject() < oo == False issue.

[rws]

So we need another ticket for

sage: bool(sqrt(2)<oo)
False

[rws]

As I have now a better overview, at least some cases could and should be fixed in Pynac. This is ​https://github.com/pynac/pynac/issues/69

[rws]

Replying to vbraun:

The symbolic constants, like pi.pyobject(), should be elements in some parent set. The symbolic constants can then coerce into the infinity ring, solving the pi.pyobject() < oo == False issue.

How would NaN be coerced? Or a future Aleph2?

[rws]

Pynac git master has a fix that does this:

sage: bool(SR(oo) > 5)
True
sage: bool(5 < SR(oo))
True
sage: bool(SR(2)<Infinity)
True
sage: bool(pi<Infinity)
True
sage: bool(pi>Infinity)
False
sage: bool(2*pi<Infinity)
True
sage: bool(SR(pi) < SR(Infinity))
True
sage: bool(sqrt(2)<oo)
True
sage: bool(log(2)<oo)
True
sage: bool(e<oo)
True
sage: bool(e+pi<oo)
True
sage: bool(e^pi<oo)
True
sage: bool(SR(2)<-oo)
False
sage: bool(SR(2)>-oo)
True
sage: bool(exp(2)>-oo)
True
sage: bool(SR(oo) > sqrt(2))
True
sage: bool(sqrt(2) < SR(oo))
True
sage: bool(SR(-oo) < sqrt(2))
True
sage: bool(sqrt(2) > SR(-oo))
True

It uses info flags and evalf where applicable. Some function info flags were introduced earlier in Pynac.

[rws]

The above is doctested in #17321 so what remains here that is not #16397?