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: sage-9.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:

Status badges

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

comment:1 Changed 4 years ago by rws

  • Branch set to u/rws/disallow_boolean_operations_with_unknown

comment:2 Changed 4 years 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 4 years 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 4 years 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 4 years 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 4 years ago by rws (previous) (diff)

comment:6 Changed 4 years 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 4 years 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 4 years 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 4 years 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 4 years 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 4 years ago by vdelecroix

  • Description modified (diff)

comment:12 Changed 4 years 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 4 years 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 4 years 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 4 years ago by rws

  • Status changed from needs_work to needs_review

comment:16 Changed 4 years 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 4 years ago by vdelecroix (previous) (diff)

comment:17 Changed 4 years ago by rws

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

comment:18 Changed 4 years 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 4 years 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 3 years 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 3 years ago by rws

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

comment:22 Changed 3 years 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 3 years ago by vdelecroix

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

comment:24 Changed 3 years 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 3 years 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 3 years ago by vdelecroix

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

comment:27 Changed 3 years 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 3 years ago by rws

  • Status changed from needs_work to needs_review

comment:29 follow-up: Changed 3 years 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 3 years 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 3 years 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 3 years 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 3 years 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 3 years 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 3 years 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 3 years 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 3 years 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 3 years 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 3 years ago by rws

  • Status changed from needs_work to needs_review

comment:40 in reply to: ↑ 37 Changed 3 years 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 3 years 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 3 years 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 3 years 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

comment:44 Changed 18 months ago by vdelecroix

  • Branch changed from u/rws/24345-1 to public/24345
  • Commit changed from aec25f266869a2f5f33e726cd6783b987588399e to cb0877143053d94e094940747624a22d30a03804
  • Dependencies #24513 deleted
  • Milestone changed from sage-8.2 to sage-9.1

rebased


New commits:

cb0877124345: fix Unknown

comment:45 Changed 18 months ago by git

  • Commit changed from cb0877143053d94e094940747624a22d30a03804 to 855f23037584d323e00f0e218a3a4a9c6e312cf1

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

f4c2c6624345: rework UnknownClass
855f23024345: adapt code in designs, graphs, etc

comment:46 Changed 17 months ago by git

  • Commit changed from 855f23037584d323e00f0e218a3a4a9c6e312cf1 to 96fd7a29c582e894465042f70c431a76e6dfe191

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

96fd7a224345: pyflakes cleaning

comment:47 Changed 17 months ago by vdelecroix

  • Status changed from needs_review to positive_review

comment:48 Changed 17 months ago by vbraun

  • Branch changed from public/24345 to 96fd7a29c582e894465042f70c431a76e6dfe191
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.