Opened 2 years ago
Closed 2 years ago
#28705 closed defect (fixed)
Fix conversion of Booleans in interfaces
Reported by:  ghmwageringel  Owned by:  

Priority:  major  Milestone:  sage9.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: 
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
andFalse
in the symbol table for them. I put the relevant code insage/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 methodbool()
for converting elements to typebool
, as well as methods__bool__()
and__nonzero__()
which are effectively aliases forbool()
. What exactly these return differs with each interface, but often it is not clearly specified. Some interfaces test whether an element isTrue
and otherwise always returnFalse
, others test for nonzeroness, 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 notFalse
rather thanTrue
, 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 thanbool()
. This ticket renames all of them tobool()
to make things consistent. A possibly more preferable solution could be to deprecatebool()
in order to get rid of the duplication.
I did not change this for Magma, as it would require nontrivial 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 nonzeroness test on a boolean:
sage: bool(fricas(True)) # error sage: bool(fricas(False)) # error
Change History (10)
comment:1 Changed 2 years ago by
 Branch set to u/ghmwageringel/28705
 Commit set to ae7af3f048608c74774e62e0405d7fd764c58ac6
 Keywords macaulay2 mathematica fricas r added
 Status changed from new to needs_review
comment:2 Changed 2 years ago by
 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 2 years ago by
It would make more sense to implement __bool__
in each subclass (faster) and make bool
an alias.
comment:4 Changed 2 years ago by
And not even an alias but
def bool(self): return bool(self)
comment:5 Changed 2 years ago by
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 2 years ago by
 Commit changed from ae7af3f048608c74774e62e0405d7fd764c58ac6 to 7665bc0a93bca53d5ee644e485a8870f6ee25ac7
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
0d4f2d8  28705: fix conversion of Booleans in interfaces

08d341d  28705: rename bool() to __bool__() in interfaces

80796ad  28705: improve default results of InterfaceElement.__bool__

5e08e35  28705: fix __bool__ for Lisp and Maxima elements

51cf310  28705: fix __bool__ for Macaulay2 elements

7665bc0  28705: implement __bool__ for Mathematica elements

comment:7 Changed 2 years ago by
 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 2 years ago by
 Reviewers set to Vincent Delecroix
 Status changed from needs_review to positive_review
comment:9 Changed 2 years ago by
Thanks.
comment:10 Changed 2 years ago by
 Branch changed from u/ghmwageringel/28705 to 7665bc0a93bca53d5ee644e485a8870f6ee25ac7
 Resolution set to fixed
 Status changed from positive_review to closed
With this branch, all tests including some optional interfaces pass on my end:
This test to check the conversions explicitly passes as well:
The changes related to optional interfaces are in separate commits, so they could be moved to separate tickets if preferred.
New commits:
28705: fix conversion of Booleans in interfaces
28705: rename __bool__() to bool() in interfaces
28705: improve default results of InterfaceElement.bool()
28705: fix bool() for Lisp and Maxima elements
28705: fix bool() for Macaulay2 elements
28705: implement bool() for Mathematica elements