Opened 4 years ago
Last modified 8 weeks ago
#22162 needs_work defect
Return Unknown from ex.is_xyz() if Pynac returns false
Reported by:  rws  Owned by:  

Priority:  major  Milestone:  sage9.4 
Component:  symbolics  Keywords:  
Cc:  pelegm  Merged in:  
Authors:  Ralf Stephan  Reviewers:  
Report Upstream:  N/A  Work issues:  
Branch:  u/rws/221621 (Commits, GitHub, GitLab)  Commit:  079597de7de7ff2f5474c1dd591942957818e4fc 
Dependencies:  Stopgaps: 
Description (last modified by )
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.
Change History (33)
comment:1 Changed 4 years ago by
 Branch set to u/rws/return_unknown_from_ex_is_xyz___if_pynac_returns_false
comment:2 Changed 4 years ago by
 Commit set to 5918af7b6f0ab73bf94a340c62fab26d2992966a
 Status changed from new to needs_review
comment:3 Changed 4 years ago by
 Description modified (diff)
comment:4 Changed 4 years ago by
 Description modified (diff)
comment:5 followups: ↓ 7 ↓ 8 Changed 4 years ago by
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?
comment:6 Changed 4 years ago by
 Commit changed from 5918af7b6f0ab73bf94a340c62fab26d2992966a to 5f5d1348d22dbfe3850f542a70afbd9c447a183c
Branch pushed to git repo; I updated commit sha1. New commits:
5f5d134  22162: refine doctests

comment:7 in reply to: ↑ 5 Changed 4 years ago by
Replying to jdemeyer:
Regarding
Unknown
, shouldn't we disallow conversion tobool
? Currently,bool(Unknown)
isFalse
andnot Unknown
isTrue
. 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).
comment:8 in reply to: ↑ 5 ; followup: ↓ 9 Changed 4 years ago by
 Description modified (diff)
Replying to jdemeyer:
Regarding
Unknown
, shouldn't we disallow conversion tobool
? Currently,bool(Unknown)
isFalse
andnot Unknown
isTrue
. What do you think about that?
Actually, what I meant is that it is fine as it is, as it preserves previous usage.
comment:9 in reply to: ↑ 8 Changed 4 years ago by
Replying to rws:
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
.
comment:10 Changed 4 years ago by
 Commit changed from 5f5d1348d22dbfe3850f542a70afbd9c447a183c to bb6f53f68ed3cd499c60e4655ba1eb8d261b38ea
Branch pushed to git repo; I updated commit sha1. New commits:
bb6f53f  22162: change more is_xyz() methods and doctests

comment:11 followup: ↓ 12 Changed 4 years ago by
As it was a nobrainer 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.
comment:12 in reply to: ↑ 11 Changed 4 years ago by
 Status changed from needs_review to needs_info
comment:13 followup: ↓ 19 Changed 4 years ago by
I think we should think about 9 before merging this. What good is Unknown
if it behaves the same as False
for practical purposes?
comment:14 followup: ↓ 15 Changed 4 years ago by
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?
comment:15 in reply to: ↑ 14 ; followup: ↓ 17 Changed 4 years ago by
Replying to rws:
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
?
comment:16 Changed 4 years ago by
There is also the purely practical point that it's easier to change the functionality of Unknown
if it's only used very little.
comment:17 in reply to: ↑ 15 ; followup: ↓ 18 Changed 4 years ago by
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
.
comment:18 in reply to: ↑ 17 Changed 4 years ago by
Replying to rws:
Semantic inconsistency:
Unknown
is meant but the output isFalse
.
With this patch, you get an object 'Unknown' which behaves a lot like False
. So this doesn't fix much...
comment:19 in reply to: ↑ 13 Changed 4 years ago by
Replying to jdemeyer:
I think we should think about 9 before merging this. What good is
Unknown
if it behaves the same asFalse
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 trivalued 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.
comment:20 Changed 3 years ago by
Are the conclusions of the above comments changed if we regard Python3 richcmp? Specifically can Py3 richcmp provide an Unknown
object that is more useful than simply being False
, e.g., can we change the behaviour of if is_xyz():
and not is_xyz()
such that the former compares with True
and the latter with False
?
comment:21 Changed 3 years ago by
Actually I find that the combinat doctest fails are fixable with a few natural changes, and so an Unknown
that behaves differently from False
seems possible.
comment:22 Changed 3 years ago by
 Branch u/rws/return_unknown_from_ex_is_xyz___if_pynac_returns_false deleted
 Commit bb6f53f68ed3cd499c60e4655ba1eb8d261b38ea deleted
 Dependencies set to #24345
 Milestone changed from sage7.5 to sage8.2
See #24345.
comment:23 Changed 3 years ago by
 Branch set to u/rws/22162
comment:24 Changed 3 years ago by
 Commit set to c86618152fad77de88f8e07c74770adc72c20e8a
 Dependencies #24345 deleted
 Description modified (diff)
 Status changed from needs_info to needs_review
New commits:
c866181  22162: Return Unknown from ex.is_xyz() if Pynac returns false

comment:25 followup: ↓ 26 Changed 3 years ago by
 Description modified (diff)
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.
comment:26 in reply to: ↑ 25 Changed 3 years ago by
Replying to rws:
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 ifis_noninteger
returns True, so both behave complementary.
What would be the point of having is_noninteger
if is_integer
is already doing the job!?
comment:27 Changed 3 years ago by
Agree. Exposing the flag to the user is not really necessary.
comment:28 Changed 3 years ago by
 Branch changed from u/rws/22162 to u/rws/221621
comment:29 Changed 3 years ago by
 Cc pelegm added
 Commit changed from c86618152fad77de88f8e07c74770adc72c20e8a to 079597de7de7ff2f5474c1dd591942957818e4fc
New commits:
079597d  22162: Return Unknown from ex.is_xyz() if Pynac returns false

comment:30 Changed 14 months ago by
 Milestone changed from sage8.2 to sage9.1
 Status changed from needs_review to needs_work
#24345 is merged in 9.1.beta5 but this one needs a rebase
comment:31 Changed 12 months ago by
 Milestone changed from sage9.1 to sage9.2
Moving tickets to milestone sage9.2 based on a review of last modification date, branch status, and severity.
comment:32 Changed 7 months ago by
 Milestone changed from sage9.2 to sage9.3
comment:33 Changed 8 weeks ago by
 Milestone changed from sage9.3 to sage9.4
Setting new milestone based on a cursory review of ticket status, priority, and last modification date.
New commits:
22162: return Unknown from is_integer()