Return Unknown from ex.is_xyz() if Pynac returns false

[rwst]

Expected:

sage: x.is_integer()
Unknown
sage: if x.is_real(): True
sage: if not x.is_real(): False
False

To clarify, I think it's possible to separate the issue of
"why do I get False" from the issue of deciding between
True/False/Unknown, by simply returning Unknown if the
Pynac logic returns false.

It would be much less surprising to the user.

Later, when the complementary flags (irreal, noninteger, nonpositive) are implemented in Pynac these function can return True/Unknown/False, e.g. is_integer will return False if is_noninteger returns True, so both behave complementary.

CC: @pelegm

Component: symbolics

Author: Ralf Stephan

Branch/Commit: u/rws/22162-1 @ 079597d

Issue created by migration from https://trac.sagemath.org/ticket/22162

[rwst]

Branch: u/rws/return_unknown_from_ex_is_xyz___if_pynac_returns_false

[rwst]

Commit: 5918af7

[rwst]

New commits:

5918af7

22162: return Unknown from is_integer()

[rwst]

Author: Ralf Stephan

[jdemeyer]

comment:5

Regarding Unknown, shouldn't we disallow conversion to bool? Currently, bool(Unknown) is False and not Unknown is True. What do you think about that?

[sagetrac-git]

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

5f5d134

22162: refine doctests

[sagetrac-git]

Changed commit from 5918af7 to 5f5d134

[rwst]

comment:7

Replying to @jdemeyer:

Regarding Unknown, shouldn't we disallow conversion to bool? Currently, bool(Unknown) is False and not Unknown is True. What do you think about that?

That would have to be changed to preserve previous usage. I don't think there is much of that in Sage (except is_zero which I don't want to deal with here).

[rwst]

comment:8

Replying to @jdemeyer:

Regarding Unknown, shouldn't we disallow conversion to bool? Currently, bool(Unknown) is False and not Unknown is True. What do you think about that?

Actually, what I meant is that it is fine as it is, as it preserves previous usage.

[jdemeyer]

comment:9

Replying to @rwst:

Actually, what I meant is that it is fine as it is, as it preserves previous usage.

If the goal of this ticket is to preserve previous usage, why bother at all? Shouldn't the goal be to fix the bad previous usage instead? That means that Unknown should be treated really as an unknown value and not as a small variation on False.

[sagetrac-git]

Changed commit from 5f5d134 to bb6f53f

[sagetrac-git]

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

bb6f53f

22162: change more is_xyz() methods and doctests

[rwst]

comment:11

As it was a no-brainer I changed the other is_xyz() function except for comparisons with zero, and I consider the ticket complete, apart from bug or doctest fixes.

Please review.

[jdemeyer]

comment:13

I think we should think about [comment:9] before merging this. What good is Unknown if it behaves the same as False for practical purposes?

[rwst]

comment:14

Well, quoting unknown.py, ..warning:: Unless PEP 335 is accepted, in the following cases, and, not and or return a somewhat wrong value::. PEP 335 was rejected so I'm being pragmatic and accept the (admittedly) small value of serving the users that don't write Sage code.

Also, isn't that an independent issue?

[jdemeyer]

comment:15

Replying to @rwst:

Also, isn't that an independent issue?

No, I think it's not an independent issue because it's the motivation of this ticket. Which problem does this ticket solve if you don't fix Unknown?

[jdemeyer]

comment:16

There is also the purely practical point that it's easier to change the functionality of Unknown if it's only used very little.

[rwst]

comment:17

Replying to @jdemeyer:

Which problem does this ticket solve if you don't fix Unknown?

Semantic inconsistency: Unknown is meant but the output is False.

[jdemeyer]

comment:18

Replying to @rwst:

Semantic inconsistency: Unknown is meant but the output is False.

With this patch, you get an object 'Unknown' which behaves a lot like False. So this doesn't fix much...

[rwst]

comment:19

Replying to @jdemeyer:

I think we should think about [comment:9] before merging this. What good is Unknown if it behaves the same as False for practical purposes?

Disclaimer: Python is not my specialty.

Either you convert Unknown to bool (and you provide the means to do it via __nonzero__) or you don't (and deny any attempt). The former is this branch at the moment, the latter would force users to explicitly compare with True/False/Unknown, disallowing if is_xyz(): conditionals. It would also disallow not is_xyz() and is_xy() and/or is_yz(). By disallow I mean an exception is raised. The patch to achieve this would simply be:

diff --git a/src/sage/misc/unknown.py b/src/sage/misc/unknown.py
index b05751e..4a6979a 100644
--- a/src/sage/misc/unknown.py
+++ b/src/sage/misc/unknown.py
@@ -66,7 +66,7 @@ class UnknownClass(UniqueRepresentation, SageObject):
             sage: not Unknown
             True
         """
-        return False
+        raise NotImplementedError('Python does not support tri-valued logic.')
 
     __nonzero__ = __bool__

Unknown is used in a few places. With the patch I get about 100 doctest fails in combinat. I have not looked further into this.

[rwst]

comment:25

Later, when the complementary flags (irreal, noninteger, nonpositive) are implemented in Pynac these function can return True/Unknown/False, e.g. is_integer will return False if is_noninteger returns True, so both behave complementary.

[videlec]

comment:26

Replying to @rwst:

Later, when the complementary flags (irreal, noninteger, nonpositive) are implemented in Pynac these function can return True/Unknown/False, e.g. is_integer will return False if is_noninteger returns True, so both behave complementary.

What would be the point of having is_noninteger if is_integer is already doing the job!?

[rwst]

comment:27

Agree. Exposing the flag to the user is not really necessary.

[rwst]

Changed branch from u/rws/22162 to u/rws/22162-1

[pelegm]

Changed commit from c866181 to 079597d

[pelegm]

New commits:

079597d

22162: Return Unknown from ex.is_xyz() if Pynac returns false

[videlec]

comment:30

#24345 is merged in 9.1.beta5 but this one needs a rebase

[mkoeppe]

comment:31

Moving tickets to milestone sage-9.2 based on a review of last modification date, branch status, and severity.

[mkoeppe]

comment:33

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

[mkoeppe]

comment:34

Setting a new milestone for this ticket based on a cursory review.