AssocOp / LatticeOp

[Alex]

There are some discrepancies between the two classes, and I'm not exactly sure why.

- AssocOp sympifies arguments in strict mode via _sympify (which is 

anyway just a proxy for sympify(., strict=True) -- is it reasonable to have a separate function

for doing just this?). Anyway, this is an issue when dealing with booleans: 

Xor(True, A) breaks as booleans are obviously not converted. LatticeOp uses sympify.

- AssocOp: the is_commutative attribute is overridden by the combination __new__()+flatten(). 

Both should probably be rewritten to take commutativity (or not) into account, this should be fully backward 

compatible, and provide more functionality for virtually nothing. 

This does not happen in LatticeOp() as _new_args_filter rightfully assumes

commutativity and does flatten()'s job. Notice that LatticeOp derives from AssocOp, and provides

as such its flatten() method. Needs refactoring?

Sorry for bugging you with these comments - I'm trying to understand what's going on :)

[Chris Smith]

Sorry for bugging you with these comments - I'm trying to understand what's going on :)

I can't help with the issues (Ronan worked on commutativity-related issues) but just want to say that such comments and questions are not a bother at all. Thanks for your interest and pursuit of this.

Chris

[Aaron Meurer]

On Fri, Jul 27, 2012 at 11:32 PM, Alex <alex.kan...@gmail.com> wrote:
> There are some discrepancies between the two classes, and I'm not exactly
> sure why.
>
> - AssocOp sympifies arguments in strict mode via _sympify (which is
> anyway just a proxy for sympify(., strict=True) -- is it reasonable to have
> a separate function
> for doing just this?). Anyway, this is an issue when dealing with booleans:
> Xor(True, A) breaks as booleans are obviously not converted. LatticeOp uses
> sympify.

Booleans won't be converted with anything, as we don't have Basic
boolean literal types.

>
> - AssocOp: the is_commutative attribute is overridden by the combination
> __new__()+flatten().
> Both should probably be rewritten to take commutativity (or not) into
> account, this should be fully backward
> compatible, and provide more functionality for virtually nothing.
> This does not happen in LatticeOp() as _new_args_filter rightfully assumes
> commutativity and does flatten()'s job. Notice that LatticeOp derives from
> AssocOp, and provides
> as such its flatten() method. Needs refactoring?

I don't think commutative even makes sense for LatticeOp. commutative
specifically refers to how the class behaves in Mul, which doesn't
make sense for booleans. LatticeOp probably derives from AssocOp
because in theory the one is a sub-type of the other (a lattice
operation is associative).

By the way, in case you haven't already started to figure it out, the
class structure here is not set in stone, or in any way claimed to be
the best (this actually goes for most features in SymPy). If you see
a problem and you think you can do it better, by all means, try it.
Our test suite will give you a good idea of if you're breaking
something or not.

Aaron Meurer

>
> Sorry for bugging you with these comments - I'm trying to understand what's
> going on :)
>

> --
> You received this message because you are subscribed to the Google Groups
> "sympy" group.
> To view this discussion on the web visit
> https://groups.google.com/d/msg/sympy/-/EYNoisCY11oJ.
> To post to this group, send email to sy...@googlegroups.com.
> To unsubscribe from this group, send email to
> sympy+un...@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/sympy?hl=en.

[Alex]

> There are some discrepancies between the two classes, and I'm not exactly
> sure why.
>
> - AssocOp sympifies arguments in strict mode via _sympify (which is
> anyway just a proxy for sympify(., strict=True) -- is it reasonable to have
> a separate function
> for doing just this?). Anyway, this is an issue when dealing with booleans:
> Xor(True, A) breaks as booleans are obviously not converted. LatticeOp uses
> sympify.

Booleans won't be converted with anything, as we don't have Basic
boolean literal types.

I'm totally fine with that. I'm just arguing that AssocOp should use simplify instead of _symplify,

and that _symplify might need to go away.

 

>
> - AssocOp: the is_commutative attribute is overridden by the combination
> __new__()+flatten().
> Both should probably be rewritten to take commutativity (or not) into
> account, this should be fully backward
> compatible, and provide more functionality for virtually nothing.
> This does not happen in LatticeOp() as _new_args_filter rightfully assumes
> commutativity and does flatten()'s job. Notice that LatticeOp derives from
> AssocOp, and provides
> as such its flatten() method. Needs refactoring?

I think we are on the same ground. I'm just seeing unnecessary code/functionality

duplication and weird inconsistencies.

 

By the way, in case you haven't already started to figure it out, the
class structure here is not set in stone, or in any way claimed to be
the best (this actually goes for most features in SymPy).  If you see
a problem and you think you can do it better, by all means, try it.
Our test suite will give you a good idea of if you're breaking
something or not.

Alright, let me submit a patch :)

 

[Alex]

Oh well, now that I'm looking more closely at the core's structure, patching what 

I wanted to initially patch makes less sense. One of the problems is that the operators

(AssocOp or LatticeOp) subclass Expr, and that Expr is really meant for other things than

logical propositions.

I guess my initial boolean simplify() will be delayed as I'm thinking about sympy.core

related issues now.

[Ronan Lamy]

Le samedi 28 juillet 2012 à 01:04 -0700, Alex a écrit :

>
>
> On Saturday, July 28, 2012 12:57:01 AM UTC-7, Aaron Meurer wrote:
> On Fri, Jul 27, 2012 at 11:32 PM, Alex
> <alex.kan...@gmail.com> wrote:
> > There are some discrepancies between the two classes, and
> I'm not exactly
> > sure why.
> >
> > - AssocOp sympifies arguments in strict mode via _sympify
> (which is
> > anyway just a proxy for sympify(., strict=True) -- is it
> reasonable to have
> > a separate function
> > for doing just this?). Anyway, this is an issue when dealing
> with booleans:
> > Xor(True, A) breaks as booleans are obviously not converted.
> LatticeOp uses
> > sympify.
>
> Booleans won't be converted with anything, as we don't have
> Basic
> boolean literal types.
>
>

> I'm totally fine with that. I'm just arguing that AssocOp should use
> simplify instead of _symplify,
> and that _symplify might need to go away.

I guess you mean sympify, not simplify. There are basically two problems
with using sympify instead of _sympify:
* sympify parses strings, which messes up the interface (we don't want
to allow 'x*y' + z), and can trigger arbitrary computation.
* sympy handles 'True' and '1' very differently, but for Python they are
nearly the same (e.g. True - 1 == 0). Letting bool objects through would
cause trouble in arithmetic contexts.

[Alex]

This does not change the problem: LatticeOp and AssocOp are inconsistent. Currently the logic module understands

and handles boolean values. Either sympy needs to consistently consider booleans as valid, first-class atoms,

either we can encapsulate these in sympy objects (kind of distasteful in my opinion).

Btw, changing _sympify to sympify does not cause any issue as far as testing is concerned, so test cases indicating wanted and unwanted

behaviors should be added. Currently, True - sympify(1) == 0 ....

[Ronan Lamy]

Le samedi 28 juillet 2012 à 10:29 -0700, Alex a écrit :
> This does not change the problem: LatticeOp and AssocOp are
> inconsistent. Currently the logic module understands
> and handles boolean values. Either sympy needs to consistently
> consider booleans as valid, first-class atoms,
> either we can encapsulate these in sympy objects (kind of distasteful
> in my opinion).

I agree about the need for consistency. However, sticking non-sympy
objects inside sympy objects is problematic (that's a long-running
debate) and True has the wrong interface anyway:
In [1]: True >> True
Out[1]: 0

So, encapsulating bools is the way to go, but it's hard, because there
is a lot of code that relies on them not being encapsulated.

> Btw, changing _sympify to sympify does not cause any issue as far as
> testing is concerned, so test cases indicating wanted and unwanted
> behaviors should be added. Currently, True - sympify(1) == 0 ....

There's nothing we can do about that, the interpreter calls int(S(1)),
which returns 1, an instance of 'int', and then computes True - 1.
On the other hand:
In [7]: True - x
---------------------------------------------------------------------------
TypeError Traceback (most recent call
last)
/home/ronan/dev/sympy/<ipython-input-7-58bde0218d6d> in <module>()
----> 1 True - x

TypeError: unsupported operand type(s) for -: 'bool' and 'Symbol'

[Alex]

Hum, interesting indeed. In the hypothetical case we accept python's True/False/None

as valid atoms, I think the following contract could work - at least in the logic module:

• No non-atomic expression can internally embed True, False or None.
  

The only expressions containing True, False None that sympy can return 

are True, False, None. This does not mean that the user can't construct expressions
containing these, they just get simplified at construction time, which is always feasible.

Obviously that does not solve the True >> True problem - but hey,

whatever we do, if True and False are the base Python objects as they should 

probably be(?), this is inevitable, so I'm fine with that.