#28705 closed defect (fixed)

Fix conversion of Booleans in interfaces

Reported by: gh-mwageringel Owned by:
Priority: major Milestone: sage-9.0
Component: interfaces Keywords: macaulay2, mathematica, fricas, r
Cc: Merged in:
Authors: Markus Wageringel Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: 7665bc0 (Commits, GitHub, GitLab) Commit: 7665bc0a93bca53d5ee644e485a8870f6ee25ac7
Dependencies: Stopgaps:

Status badges

Description

This ticket deals with several issues related to the conversion of Booleans from Sage to Pexpect interfaces and back.

Conversion from Sage to interfaces

This fails for several interfaces, those in which Booleans are not represented by integers 0 and 1.

sage: giac._true_symbol()
'true'
sage: giac('true')
true
sage: giac(True)  # should be true
1

This happens because bool is a subtype of int, which is not accounted for in the conversion routine in the parent class Interface. This can be fixed in interfaces/interface.py for all the affected interfaces, which are at least: Gap, Giac, FriCAS, Lisp, Macaulay2, Mathematica, Maxima, R (and possibly Magma, Maple, Kash).

Conversion from interfaces to Sage

This seems to be the responsibility of each individual interface. This ticket only deals with some of them.

  • Giac, Mathematica and Maxima use Pynac for converting symbols to Sage, so this ticket registers True and False in the symbol table for them. I put the relevant code in sage/symbolic/constants.py, mainly due to lack of a better place. Booleans are not really symbolic mathematical constants – I hope this change still makes sense.
  • The parent class InterfaceElement defines a method bool() for converting elements to type bool, as well as methods __bool__() and __nonzero__() which are effectively aliases for bool(). What exactly these return differs with each interface, but often it is not clearly specified. Some interfaces test whether an element is True and otherwise always return False, others test for non-zeroness, in part for historic reasons.

It appears safe to assume that bool() should at least convert Booleans correctly from an interface to Sage, so I adjusted the implementation for some interfaces where this seemed to be a problem: Lisp, Maxima, Macaulay2 and Mathematica. I do not have much experience with Lisp and Maxima, so am not sure how useful this change is for them.

  • I changed the default implementation of InterfaceElement.bool() so that it tests whether an element is not False rather than True, solely to make it less surprising and more pythonic when applied to elements that are not Booleans. For example:
    giac('"a"')  # previously this returned false
    true
    
  • Some interfaces only overwrite __bool__(), causing it to return different results than bool(). This ticket renames all of them to bool() to make things consistent. A possibly more preferable solution could be to deprecate bool() in order to get rid of the duplication.

I did not change this for Magma, as it would require non-trivial changes I cannot test, nor for FriCAS, as I do not know how to implement it. Note that the following still fails as it attempts a non-zeroness test on a boolean:

sage: bool(fricas(True))  # error
sage: bool(fricas(False))  # error

Change History (10)

comment:1 Changed 20 months ago by gh-mwageringel

  • Authors set to Markus Wageringel
  • Branch set to u/gh-mwageringel/28705
  • Commit set to ae7af3f048608c74774e62e0405d7fd764c58ac6
  • Keywords macaulay2 mathematica fricas r added
  • Status changed from new to needs_review

With this branch, all tests including some optional interfaces pass on my end:

./sage -bt -l -p 8 --optional=sage,macaulay2,mathematica,fricas --logfile=logs/ptestlongopt.log src/sage

This test to check the conversions explicitly passes as well:

from sage.interfaces.all import *
assert all((repr(s(True)) == s._true_symbol() and
            repr(s(False)) == s._false_symbol() and
            s(True).bool() is True and
            s(False).bool() is False) for s in [gap, giac, gp, lisp, macaulay2, mathematica, maxima, r, fricas])

The changes related to optional interfaces are in separate commits, so they could be moved to separate tickets if preferred.


New commits:

d73a2df28705: fix conversion of Booleans in interfaces
3d9969e28705: rename __bool__() to bool() in interfaces
fcf1caa28705: improve default results of InterfaceElement.bool()
3e91be128705: fix bool() for Lisp and Maxima elements
62a53e728705: fix bool() for Macaulay2 elements
ae7af3f28705: implement bool() for Mathematica elements

comment:2 Changed 19 months ago by vdelecroix

  • Status changed from needs_review to needs_info

__bool__ is the Python converter to boolean, see Python documentation (and __nonzero__ is the one in Python 2). I don't understand why you would remove it. Without it, the following would not work properly.

if my_object:   # implicit call to __bool__
   ...

comment:3 Changed 19 months ago by vdelecroix

It would make more sense to implement __bool__ in each subclass (faster) and make bool an alias.

comment:4 Changed 19 months ago by vdelecroix

And not even an alias but

def bool(self):
    return bool(self)

comment:5 Changed 19 months ago by gh-mwageringel

That is a good point. I merely followed what was laid out in the superclass InterfaceElement: it defines the method bool and implements __bool__ by redirecting to it, so overriding bool rather than __bool__ in the subclasses seemed natural at first.

Though I agree that it would be better to do it the other way around, as you suggested. In the long run, this would also make it easier to deprecate the method bool which is redundant in my opinion. I will update the branch accordingly.

comment:6 Changed 19 months ago by git

  • Commit changed from ae7af3f048608c74774e62e0405d7fd764c58ac6 to 7665bc0a93bca53d5ee644e485a8870f6ee25ac7

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

0d4f2d828705: fix conversion of Booleans in interfaces
08d341d28705: rename bool() to __bool__() in interfaces
80796ad28705: improve default results of InterfaceElement.__bool__
5e08e3528705: fix __bool__ for Lisp and Maxima elements
51cf31028705: fix __bool__ for Macaulay2 elements
7665bc028705: implement __bool__ for Mathematica elements

comment:7 Changed 19 months ago by gh-mwageringel

  • Status changed from needs_info to needs_review

I have updated the branch as suggested, that is:

  • in InterfaceElement, bool() is now a wrapper for __bool__(),
  • in the subclasses, bool() was renamed to __bool__() and __nonzero__ was added where applicable.

Everything else has stayed the same.

comment:8 Changed 19 months ago by vdelecroix

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to positive_review

comment:9 Changed 19 months ago by gh-mwageringel

Thanks.

comment:10 Changed 19 months ago by vbraun

  • Branch changed from u/gh-mwageringel/28705 to 7665bc0a93bca53d5ee644e485a8870f6ee25ac7
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.