Opened 18 months ago

Last modified 15 months ago

#24345 needs_review enhancement

Disallow boolean operations with Unknown

Reported by: rws Owned by:
Priority: major Milestone: sage-8.2
Component: misc Keywords:
Cc: Merged in:
Authors: Ralf Stephan Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: u/rws/24345-1 (Commits) Commit: aec25f266869a2f5f33e726cd6783b987588399e
Dependencies: #24513 Stopgaps:

Description (last modified by vdelecroix)

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 (43)

comment:1 Changed 18 months ago by rws

  • Branch set to u/rws/disallow_boolean_operations_with_unknown

comment:2 Changed 18 months ago by rws

  • Authors set to Ralf Stephan
  • Commit set to 2171a3e32191c58c9dbe0733db3f171e47fc851f
  • Status changed from new to needs_review

New commits:

4d60cb624345: changes to Unknown
2171a3e24345: changes to code using Unknown

comment:3 follow-ups: Changed 18 months ago by 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 treating Unknown as false.

comment:4 in reply to: ↑ 3 Changed 18 months ago by rws

Replying to jdemeyer:

To me, requiring people to write foo() is True is a bigger problem than treating Unknown as false.

So you would not argue against a ticket that uses such an Unknown?

comment:5 in reply to: ↑ 3 Changed 18 months ago by rws

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 treating Unknown 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?

Last edited 18 months ago by rws (previous) (diff)

comment:6 Changed 18 months ago by jdemeyer

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 follow-up: Changed 18 months ago by rws

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 18 months ago by rws

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 18 months ago by jdemeyer

Replying to rws:

Can we agree on this?

Well, first I have to agree with myself. I read the discussion again on #22162 and I'm less convinced about my own reasoning there. I have not thought much about it now, but my feeling is that I was wrong on #22162.

comment:10 in reply to: ↑ 3 Changed 18 months ago by vdelecroix

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 17 months ago by vdelecroix

  • Description modified (diff)

comment:12 Changed 17 months ago by vdelecroix

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 17 months ago by vdelecroix

  • 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 17 months ago by git

  • Commit changed from 2171a3e32191c58c9dbe0733db3f171e47fc851f to ed8ef17484c23cb199776762527df747cf7a59e6

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

246be61Merge branch 'develop' into t/24345/disallow_boolean_operations_with_unknown
ed8ef1724345: fix

comment:15 Changed 17 months ago by rws

  • Status changed from needs_work to needs_review

comment:16 Changed 17 months ago by vdelecroix

  • 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]])
Last edited 17 months ago by vdelecroix (previous) (diff)

comment:17 Changed 17 months ago by rws

  • Branch changed from u/rws/disallow_boolean_operations_with_unknown to u/rws/24345

comment:18 Changed 17 months ago by rws

  • Commit changed from ed8ef17484c23cb199776762527df747cf7a59e6 to d71aabf81a69e35ff1a3f95658cfe78ddbe98cb9
  • Status changed from needs_work to needs_review

New commits:

a6f0ded24513: fix difference family
d71aabf24345: Disallow boolean operations with Unknown

comment:19 Changed 17 months ago by vdelecroix

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 16 months ago by vdelecroix

  • 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 16 months ago by rws

  • Branch changed from u/rws/24345 to u/rws/24345-1

comment:22 Changed 16 months ago by rws

  • Commit changed from d71aabf81a69e35ff1a3f95658cfe78ddbe98cb9 to f916205b0bcab16501053a70fa317c7b3b415aa8
  • Status changed from needs_work to needs_review

New commits:

f91620524345: Disallow boolean operations with Unknown

comment:23 Changed 16 months ago by vdelecroix

You break bool(Unknown) and this is not mentioned.

comment:24 Changed 16 months ago by vdelecroix

  • 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 16 months ago by vdelecroix

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 16 months ago by vdelecroix

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_info to needs_work

comment:27 Changed 16 months ago by git

  • Commit changed from f916205b0bcab16501053a70fa317c7b3b415aa8 to 601d8f9711d7235af64cdd1c38daf47deb483a07

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

601d8f924345: fixes

comment:28 Changed 16 months ago by rws

  • Status changed from needs_work to needs_review

comment:29 follow-up: Changed 16 months ago by vdelecroix

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 ; follow-up: Changed 16 months ago by vdelecroix

Replying to vdelecroix:

You should also test True | Unknown as well as False | 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 16 months ago by git

  • Commit changed from 601d8f9711d7235af64cdd1c38daf47deb483a07 to 712ba5e8dc2d84f8ca8ae636b849b144d13f914d

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

f95aa51Merge branch 'develop' into t/24345/24345-1
712ba5e24345: fixes

comment:32 in reply to: ↑ 30 ; follow-up: Changed 16 months ago by 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

comment:33 in reply to: ↑ 32 ; follow-up: Changed 16 months ago by vdelecroix

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 16 months ago by rws

Replying to vdelecroix:

Precisely: when x.__or__(y) returns NotImplemented Python automatically calls y.__or__(x).

Then why does this mechanism not work with x=True and y=Unknown?

comment:35 Changed 16 months ago by jdemeyer

  • 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 16 months ago by vdelecroix

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 follow-up: Changed 16 months ago by 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 with Unknown I will remove these methods.

comment:38 Changed 16 months ago by git

  • Commit changed from 712ba5e8dc2d84f8ca8ae636b849b144d13f914d to 8b4ea3df9afe15f915f499a5b9c92bc454812e5b

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

8b4ea3d24345: fixes, cosmetics

comment:39 Changed 16 months ago by rws

  • Status changed from needs_work to needs_review

comment:40 in reply to: ↑ 37 Changed 16 months ago by vdelecroix

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 with Unknown 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 16 months ago by vdelecroix

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 16 months ago by git

  • Commit changed from 8b4ea3df9afe15f915f499a5b9c92bc454812e5b to e293dba0b16d076877688d0576dddd5e05d49656

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

e293dba24345: more cosmetics

comment:43 Changed 15 months ago by git

  • Commit changed from e293dba0b16d076877688d0576dddd5e05d49656 to aec25f266869a2f5f33e726cd6783b987588399e

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

aec25f2Merge branch 'develop' into t/24345/24345-1
Note: See TracTickets for help on using tickets.