Disallow boolean operations with Unknown

[rwst]

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

Component: misc

Author: Ralf Stephan

Branch/Commit: 96fd7a2

Reviewer: Vincent Delecroix

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

[rwst]

Branch: u/rws/disallow_boolean_operations_with_unknown

[rwst]

Commit: 2171a3e

[rwst]

New commits:

4d60cb6	24345: changes to Unknown
2171a3e	24345: changes to code using Unknown

[rwst]

Author: Ralf Stephan

[jdemeyer]

comment:3

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.

[rwst]

comment:4

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?

[rwst]

comment:5

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?

[jdemeyer]

comment:6

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.

[rwst]

comment:7

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?

[rwst]

comment:8

Also after the flag query functions return unknown changes can be made that make them return True/Unknown/False where False really means False.

[jdemeyer]

comment:9

Replying to @rwst:

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.

[videlec]

comment:10

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.

[videlec]

comment:12

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!

[videlec]

comment:13

one doctest failure (see patchbot report)

sage -t --long src/sage/graphs/strongly_regular_db.pyx  # 1 doctest failed

[sagetrac-git]

Changed commit from 2171a3e to ed8ef17

[sagetrac-git]

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

246be61	Merge branch 'develop' into t/24345/disallow_boolean_operations_with_unknown
ed8ef17	24345: fix

[videlec]

Dependencies: #24513

[videlec]

comment:16

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]])

[rwst]

Changed branch from u/rws/disallow_boolean_operations_with_unknown to u/rws/24345

[jdemeyer]

comment:35

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.

[videlec]

comment:36

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));
}

[rwst]

comment:37

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.

[sagetrac-git]

Changed commit from 712ba5e to 8b4ea3d

[sagetrac-git]

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

8b4ea3d

24345: fixes, cosmetics

[videlec]

comment:40

Replying to @rwst:

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++).

[videlec]

comment:41

And since we are at refactoring, I don't understand why Unknown should inherit from SageObject. It does not interact with anything else.

[sagetrac-git]

Changed commit from 8b4ea3d to e293dba

[sagetrac-git]

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

e293dba

24345: more cosmetics

[sagetrac-git]

Changed commit from e293dba to aec25f2

[sagetrac-git]

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

aec25f2

Merge branch 'develop' into t/24345/24345-1

[videlec]

Changed dependencies from #24513 to none

[videlec]

Changed commit from aec25f2 to cb08771

[videlec]

Changed branch from u/rws/24345-1 to public/24345

[videlec]

comment:44

rebased

---

New commits:

cb08771

24345: fix Unknown

[sagetrac-git]

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

f4c2c66	24345: rework UnknownClass
855f230	24345: adapt code in designs, graphs, etc

[sagetrac-git]

Changed commit from cb08771 to 855f230

[sagetrac-git]

Changed commit from 855f230 to 96fd7a2

[sagetrac-git]

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

96fd7a2

24345: pyflakes cleaning

[vbraun]

Changed branch from public/24345 to 96fd7a2