Opened 4 years ago
Closed 17 months ago
#24345 closed enhancement (fixed)
Disallow boolean operations with Unknown
Reported by:  rws  Owned by:  

Priority:  major  Milestone:  sage9.1 
Component:  misc  Keywords:  
Cc:  Merged in:  
Authors:  Ralf Stephan  Reviewers:  Vincent Delecroix 
Report Upstream:  N/A  Work issues:  
Branch:  96fd7a2 (Commits, GitHub, GitLab)  Commit:  96fd7a29c582e894465042f70c431a76e6dfe191 
Dependencies:  Stopgaps: 
Description (last modified by )
The Unknown
from misc/unknown.py
incorrectly behaves like False
in the context of boolean operations. It is mostly useless as long as Python has no tristate conditionals and logic operators. This ticket raises an error instead.
As a consequence of this modification, Sage code is adapted as follows for a given tristate troolean
if troolean is True: ... elif troolean is False: ... else: ...
Change History (48)
comment:1 Changed 4 years ago by
 Branch set to u/rws/disallow_boolean_operations_with_unknown
comment:2 Changed 4 years ago by
 Commit set to 2171a3e32191c58c9dbe0733db3f171e47fc851f
 Status changed from new to needs_review
comment:3 followups: ↓ 4 ↓ 5 ↓ 10 Changed 4 years ago by
I'm not sure that there is actually a bug here...
To me, requiring people to write foo() is True
is a bigger problem than treating Unknown
as false.
comment:4 in reply to: ↑ 3 Changed 4 years ago by
Replying to jdemeyer:
To me, requiring people to write
foo() is True
is a bigger problem than treatingUnknown
as false.
So you would not argue against a ticket that uses such an Unknown?
comment:5 in reply to: ↑ 3 Changed 4 years ago by
Replying to jdemeyer:
I'm not sure that there is actually a bug here...
To me, requiring people to write
foo() is True
is a bigger problem than treatingUnknown
as false.
Another alternative with this ticket's Unknown
would be to raise an UnknownError
instead of NotImplementedError
and suggest to the user to catch it. This way you can still write if foo():
. What about it?
comment:6 Changed 4 years ago by
Sorry, I'm not quite following what you are trying to say...
Maybe you should explain better which bug this ticket is meant to address any why it is a bug.
comment:7 followup: ↓ 9 Changed 4 years ago by
The bug is that the flag query functions is_integer, is_real etc return False instead of Unknown, ie #22162. I followed your argument there that using Unknown as is would win nothing, although I think it would be less confusing. This ticket tries to implement an Unknown that behaves more like what we want. Can we agree on this? Have I misunderstood something?
comment:8 Changed 4 years ago by
Also after the flag query functions return unknown changes can be made that make them return True/Unknown/False? where False really means False.
comment:9 in reply to: ↑ 7 Changed 4 years ago by
comment:10 in reply to: ↑ 3 Changed 4 years ago by
Unknown
is broken. The only reasonable proposals are in order of decreasing preference (to me):
 make Python troolean compatible
 the proposal of this ticket
 remove completely
Unknown
I definitely support the changes from this ticket.
comment:11 Changed 4 years ago by
 Description modified (diff)
comment:12 Changed 4 years ago by
In an ideal world, we would have the following behavior
if troolean: # here troolean = True elif not troolean: # here troolean = False else: # here troolean = Unknown
It would be so much better to patch Python!
comment:13 Changed 4 years ago by
 Status changed from needs_review to needs_work
one doctest failure (see patchbot report)
sage t long src/sage/graphs/strongly_regular_db.pyx # 1 doctest failed
comment:14 Changed 4 years ago by
 Commit changed from 2171a3e32191c58c9dbe0733db3f171e47fc851f to ed8ef17484c23cb199776762527df747cf7a59e6
comment:15 Changed 4 years ago by
 Status changed from needs_work to needs_review
comment:16 Changed 4 years ago by
 Dependencies set to #24513
 Status changed from needs_review to needs_work
From the documentation of designs.differenf_family
 ``existence``  if ``True``, then return either ``True`` if Sage knows how to build such design, ``Unknown`` if it does not and ``False`` if it knows that the design does not exist.
Hence, the isinstance(ret, tuple)
should not be needed in
+ ....: ret = designs.difference_family(v,k,l,existence=True) + ....: if ret is True or isinstance(ret, tuple):
There is indeed a bug in the design code (see #24513)
sage: designs.difference_family(3,2,1, existence=True) # should be True (Ring of integers modulo 3, [[1, 2]])
comment:17 Changed 4 years ago by
 Branch changed from u/rws/disallow_boolean_operations_with_unknown to u/rws/24345
comment:18 Changed 4 years ago by
 Commit changed from ed8ef17484c23cb199776762527df747cf7a59e6 to d71aabf81a69e35ff1a3f95658cfe78ddbe98cb9
 Status changed from needs_work to needs_review
comment:19 Changed 4 years ago by
Looks good.
Could you document more carefully the behavior of Unknown
class? Especially in the design and stronly regular graph codes where you need to write
my_function(params, existence=True) is True
and not simply
my_function(params, existence=True)
comment:20 Changed 3 years ago by
 Status changed from needs_review to needs_work
I like the idea of Jereon from comment:5. It would be clearer with UnknownError
rather than the less specific NotImplementedError
. And this would allow the alternative troolean evaluation
try: if troolean: ... else: .... except UnknownError: ....
And as said from comment:19. Documentation is needed.
comment:21 Changed 3 years ago by
 Branch changed from u/rws/24345 to u/rws/243451
comment:22 Changed 3 years ago by
 Commit changed from d71aabf81a69e35ff1a3f95658cfe78ddbe98cb9 to f916205b0bcab16501053a70fa317c7b3b415aa8
 Status changed from needs_work to needs_review
New commits:
f916205  24345: Disallow boolean operations with Unknown

comment:23 Changed 3 years ago by
You break bool(Unknown)
and this is not mentioned.
comment:24 Changed 3 years ago by
 Status changed from needs_review to needs_info
Why this change
@@ 168,7 +213,7 @@ class UnknownClass(UniqueRepresentation, SageObject): if other is self: return rich_to_bool(op, 0) if not isinstance(other, bool):  return NotImplemented + return Unknown if other: return rich_to_bool(op, 1) else:
comment:25 Changed 3 years ago by
The following should not be broken
@@ 114,14 +153,18 @@ class UnknownClass(UniqueRepresentation, SageObject): sage: Unknown  False Unknown sage: Unknown  Unknown  Unknown + Traceback (most recent call last): + ... + UnknownError: Unknown does not evaluate in boolean context sage: Unknown  True True
(as well as operation &
). The implementation of __xor__
and __and__
should be modified.
comment:26 Changed 3 years ago by
 Reviewers set to Vincent Delecroix
 Status changed from needs_info to needs_work
comment:27 Changed 3 years ago by
 Commit changed from f916205b0bcab16501053a70fa317c7b3b415aa8 to 601d8f9711d7235af64cdd1c38daf47deb483a07
Branch pushed to git repo; I updated commit sha1. New commits:
601d8f9  24345: fixes

comment:28 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:29 followup: ↓ 30 Changed 3 years ago by
You should also test True  Unknown
as well as False  Unknown
.
__not__
is by no way a special Python method. It would better be removed.
comment:30 in reply to: ↑ 29 ; followup: ↓ 32 Changed 3 years ago by
Replying to vdelecroix:
You should also test
True  Unknown
as well asFalse  Unknown
.
which is also broken
sage: False  Unknown Traceback (most recent call last): ... TypeError: unsupported operand type(s) for : 'bool' and 'UnknownClass' sage: True  Unknown Traceback (most recent call last): ... TypeError: unsupported operand type(s) for : 'bool' and 'UnknownClass'
comment:31 Changed 3 years ago by
 Commit changed from 601d8f9711d7235af64cdd1c38daf47deb483a07 to 712ba5e8dc2d84f8ca8ae636b849b144d13f914d
comment:32 in reply to: ↑ 30 ; followup: ↓ 33 Changed 3 years ago by
Replying to vdelecroix:
which is also broken
sage: False  Unknown Traceback (most recent call last): ... TypeError: unsupported operand type(s) for : 'bool' and 'UnknownClass'
I have no idea how to fix this so I just added a test. This is odd:
sage: True  1 1 sage: True.__or__(1) NotImplemented
comment:33 in reply to: ↑ 32 ; followup: ↓ 34 Changed 3 years ago by
Replying to rws:
Replying to vdelecroix:
which is also broken
sage: False  Unknown Traceback (most recent call last): ... TypeError: unsupported operand type(s) for : 'bool' and 'UnknownClass'I have no idea how to fix this so I just added a test. This is odd:
sage: True  1 1 sage: True.__or__(1) NotImplemented
Precisely: when x.__or__(y)
returns NotImplemented
Python automatically calls y.__or__(x)
.
comment:34 in reply to: ↑ 33 Changed 3 years ago by
Replying to vdelecroix:
Precisely: when
x.__or__(y)
returnsNotImplemented
Python automatically callsy.__or__(x)
.
Then why does this mechanism not work with x=True and y=Unknown?
comment:35 Changed 3 years ago by
 Status changed from needs_review to needs_work
PEP 335 was rejected, so can we please stop pretending that it will eventually be accepted? In other words: remove all references to PEP 335.
comment:36 Changed 3 years ago by
For reference, here is the implementation for __or__
for booleans
static PyObject * bool_or(PyObject *a, PyObject *b) { if (!PyBool_Check(a)  !PyBool_Check(b)) return PyLong_Type.tp_as_number>nb_or(a, b); return PyBool_FromLong((a == Py_True)  (b == Py_True)); }
comment:37 followup: ↓ 40 Changed 3 years ago by
OK, now I get it. The members __and__
and __or__
have nothing to do with logic operators, they are associated with bitwise, i.e. &
and 
. The original author added these methods thinking that &
and 
are logical (as seen in the method description) but they are not and as I don't think bit operations have a meaning with Unknown
I will remove these methods.
comment:38 Changed 3 years ago by
 Commit changed from 712ba5e8dc2d84f8ca8ae636b849b144d13f914d to 8b4ea3df9afe15f915f499a5b9c92bc454812e5b
Branch pushed to git repo; I updated commit sha1. New commits:
8b4ea3d  24345: fixes, cosmetics

comment:39 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:40 in reply to: ↑ 37 Changed 3 years ago by
Replying to rws:
OK, now I get it. The members
__and__
and__or__
have nothing to do with logic operators, they are associated with bitwise, i.e.&
and
. The original author added these methods thinking that&
and
are logical (as seen in the method description) but they are not and as I don't think bit operations have a meaning withUnknown
I will remove these methods.
Note that the operators and
and or
are only using __nonzero__
. Though they are not logical operator in the most common sense (like &&
and 
in C/C++).
comment:41 Changed 3 years ago by
And since we are at refactoring, I don't understand why Unknown
should inherit from SageObject
. It does not interact with anything else.
comment:42 Changed 3 years ago by
 Commit changed from 8b4ea3df9afe15f915f499a5b9c92bc454812e5b to e293dba0b16d076877688d0576dddd5e05d49656
Branch pushed to git repo; I updated commit sha1. New commits:
e293dba  24345: more cosmetics

comment:43 Changed 3 years ago by
 Commit changed from e293dba0b16d076877688d0576dddd5e05d49656 to aec25f266869a2f5f33e726cd6783b987588399e
Branch pushed to git repo; I updated commit sha1. New commits:
aec25f2  Merge branch 'develop' into t/24345/243451

comment:44 Changed 18 months ago by
 Branch changed from u/rws/243451 to public/24345
 Commit changed from aec25f266869a2f5f33e726cd6783b987588399e to cb0877143053d94e094940747624a22d30a03804
 Dependencies #24513 deleted
 Milestone changed from sage8.2 to sage9.1
comment:45 Changed 18 months ago by
 Commit changed from cb0877143053d94e094940747624a22d30a03804 to 855f23037584d323e00f0e218a3a4a9c6e312cf1
comment:46 Changed 17 months ago by
 Commit changed from 855f23037584d323e00f0e218a3a4a9c6e312cf1 to 96fd7a29c582e894465042f70c431a76e6dfe191
Branch pushed to git repo; I updated commit sha1. New commits:
96fd7a2  24345: pyflakes cleaning

comment:47 Changed 17 months ago by
 Status changed from needs_review to positive_review
comment:48 Changed 17 months ago by
 Branch changed from public/24345 to 96fd7a29c582e894465042f70c431a76e6dfe191
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
24345: changes to Unknown
24345: changes to code using Unknown