Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relationals and BooleanAtom #159

Merged
merged 3 commits into from
Jul 3, 2017
Merged

Conversation

ShikharJ
Copy link
Member

Please review and suggest improvements.

if self == BooleanTrue:
return sympy.S.true
else:
return sympy.S.false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SymPy has two classes.

In [5]: sympy.S.false.__class__.__mro__
Out[5]: 
(sympy.logic.boolalg.BooleanFalse,
 sympy.logic.boolalg.BooleanAtom,
 sympy.logic.boolalg.Boolean,
 sympy.core.basic.Basic,
 object)

DictBasic, symarray, series, diff, zeros, eye, diag, ones,
Derivative, Subs, add, expand, has_symbol, UndefFunction,
Function, FunctionSymbol as AppliedUndef)
zoo, nan, BooleanTrue, BooleanFalse, have_mpfr, have_mpc,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can add these to the bottom of the list instead of updating all the lines

@ShikharJ
Copy link
Member Author

@isuruf What can be done to fix the build failure?

@isuruf
Copy link
Member

isuruf commented Jun 17, 2017

See for example, https://github.com/ShikharJ/symengine.py/blob/52fb3d2d7a8b7925b9207415178af7404739fdd5/symengine/lib/symengine.pxd#L463

@ShikharJ
Copy link
Member Author

Ping @isuruf.

pass


class BooleanTrue(Boolean):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have a common parent BooleanAtom for these 2 classes.

Ne = Unequality


class GreaterThan(Relational):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the need here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's for SymPy compatibility, for the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then make it a function, otherwise isinstance(GreaterThan(x, y), GreaterThan) will return false instead of throwing an error

s = self.args_as_sage()
return sage.le(*s)

func = __class__
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd remove this as Ge(x, y).func(x, y) would return Le(x, y)

@@ -7,7 +7,7 @@
MatrixBase, Basic, Lambdify, LambdifyCSE, Lambdify as lambdify,
DictBasic, symarray, series, diff, zeros, eye, diag, ones,
Derivative, Subs, add, expand, has_symbol, UndefFunction,
Function, FunctionSymbol as AppliedUndef)
Function, FunctionSymbol as AppliedUndef, true, false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can add the relationals classes like Eq, Equality here, ... if they are available in when you do from sympy import *

def _sage_(self):
import sage.all as sage
s = self.args_as_sage()
return sage.gt(*s)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work?


assert pi == sympify(sage.pi)
assert E == sympify(sage.e)
assert oo == sympify(sage.oo)
assert zoo == sympify(sage.unsigned_infinity)
assert nan == sympify(sage.NaN)
assert true == sympify(True)
assert false == sympify(False)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test doesn't belong in this file

@isuruf
Copy link
Member

isuruf commented Jun 17, 2017

In SymPy x < y returns a less than object. We need to do the same

@ShikharJ ShikharJ force-pushed the Relationals branch 2 times, most recently from a415b27 to 46bfba5 Compare June 18, 2017 23:32
@ShikharJ
Copy link
Member Author

@isuruf I couldn't find where this was being parsed in SymPy. Can you point that out to me?

@isuruf
Copy link
Member

isuruf commented Jun 19, 2017

In pure python you need to define __lt__. For Cython, you need to use __richcmp__

@ShikharJ
Copy link
Member Author

@isuruf Are the current changes satisfactory? Or should __lt__ and __richcmp__ be overloaded in all of the classes?

@ShikharJ
Copy link
Member Author

Ping @isuruf.

@isuruf
Copy link
Member

isuruf commented Jun 20, 2017

Looks good. I'll merge this after I've done a release over the weekend.

@ShikharJ ShikharJ force-pushed the Relationals branch 2 times, most recently from 594e8af to 8ea8e6b Compare June 22, 2017 13:41
@ShikharJ
Copy link
Member Author

@isuruf Can this be merged? It would be required for Logic classes.

@isuruf
Copy link
Member

isuruf commented Jun 28, 2017

@ShikharJ, if you need this PR for other PRs, please send the new PR with the commits here. I'll merge this after the release.

@isuruf isuruf merged commit 6deb3b1 into symengine:master Jul 3, 2017
@ShikharJ ShikharJ deleted the Relationals branch July 3, 2017 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants