bastikr / boolean.py

Implements boolean algebra in one module.
BSD 2-Clause "Simplified" License
78 stars 34 forks source link

Remove __new__ #33

Closed Kronuz closed 8 years ago

Kronuz commented 8 years ago

In issue #32, @bastikr said:

I agree let's remove the __new__ functions. The only one we might consider keeping is the one for Function since it checks that the number of arguments is correct.

So, this issue is for eventually removing all __new__() that can be removed.

pombredanne commented 8 years ago

I am playing with this a bit. But I am struggling to get these removed alright.

pombredanne commented 8 years ago

Well, the hierarchy of metaclasses has something recursive to it. The Expression needs an algebra which is defined by Expressions subclasses. I cannot find a clean way yet to untangle this: but why would I want to do this? We discussed some of it in #30. But for me the main issue is #36 : I cannot find a clean simple way to do this except by forking the code and customize it to my liking for having more common stringification using the operators I want. This is the class hierarchy today:

        Expression
            BaseElement
                _FALSE
                _TRUE
            Symbol
            Function
                NOT
                DualBase
                    AND
                    OR

Hidden in the there is the fact that TRUE and FALSE are also BaseElement._TRUE() and BaseElement._FALSE() singletons respectively and attributes of Expression.

And that __new__ is overriden throughout to deal with several special cases of 0/1True/False : I think that if these specials were removed and to be handled by the parser instead, we can likely get rid of most extraneous __new__. With that case supported, I am kinda stuck. I need to move on... and I would rather not maintain a separate branch if I do not have to. Note I could put my suggestions in a branch too. Let me take a crack at this.

Any ideas on how to untangle this?

pombredanne commented 8 years ago

I also think that supporting anonymous symbols makes things complex ... I cannot fathom the use case for a free standing expression with anonymous symbols? what would it be?

pombredanne commented 8 years ago

And I cannot see a reason beside optimization to have TRUE and FALSE being singletons. Any comments?

Kronuz commented 8 years ago

It's probably just that you you might want to compare the results against TRUE or FALSE using "is", as in: assert expr.simplify() is TRUE, and we probably should have a test about it.

pombredanne commented 8 years ago

@Kronuz I get that but the singleton is not even used everywhere consistently. I am getting rid of it being a singleton which makes the code more complex.

pombredanne commented 8 years ago

Being able to use boolean expression just based on a bare import does not make sense. Expressions can only exist with certain defaults classes for TRUE/FALSE/AND/OR/NOT/Symbol/tokenizer. We can make these the defaults but not global defaults. So rather than using __new__ I will be introducing an explicit factory to customize/configure an algebra to keep thing clean and easier to process and understand

pombredanne commented 8 years ago

I am also making parse a method of Expression. Parsing outside of the context of an algebra does not make sense

pombredanne commented 8 years ago

I also thing that combining simplification in creation make things overly complex. I would be in favour of removing the simplify=True arg from all constructors and instead call simplify() explicitly when needed. Any objections?

pombredanne commented 8 years ago

I am also simplifying using plain attributes rather than properties. In most cases these are not needed and make the code more complex for no good reasons. We are all consenting adults, and trying to shield from mutability with properties is not needed IMHO. Any objections?

pombredanne commented 8 years ago

I wrote:

I also think that supporting anonymous symbols makes things complex ... I cannot fathom the use case for a free standing expression with anonymous symbols? what would it be?

I am removing support for these in my explorations

pombredanne commented 8 years ago

Supporting strings, 0, 1, False, True and None as possible expression literals is making the code quite complex.... I do not see mucho value in this. Any comments? I am removing support for these in my explorations in favor of using instances of TRUE and FALSE exclusively when declaring expressions in Python.

pombredanne commented 8 years ago

I think I grok now the root of the problems here: subclassing. Symbol and BaseElement should not be IMHO subclasses of Expression. These are literals, terminals, leaves. And probably neither should Function. An expression contains a collection of these. But a Function alone is not a type of expression, this is a part of. And while an expression can be composed of a single Symbol and BaseElement these are also a part of and a type of expression. So subclassing everything from Expression does make sense to avoid duplicating code, but does not make sense logically IMHO.

There should be IMHO:

  1. an algebra defined by the implementation of the base elements, functions, symbol and tokenizer
  2. from a defined algebra you can build expressions. And expressions holds a tree of base elements, functions, symbol.
  3. While functions have arguments, Symbols and base elements not. So expressions do not have have arguments. They hold a single expression root object, not a sequence of args.
  4. Symbols have an associated object, but Expressions, Functions and BaseElements do not (though eventually a symbol and base element are the same: and the associated object of a base element is True or False. And Symbols could be defined as being TRUE or FALSE too possibly, as opposed to only True today.
  5. NOT has a single argument, and AND and OR have a sequence of arguments. Each with markedly different methods.

I may be restating mostly the obvious here.... the main point is that subclassing everything from Expression may not make sense and may be mixing apple and oranges in a the same type. I will try to see if injection some composition and a different class structure may help.

The second point is that to allow any kind of customization, you cannot use the bare classes right away. You need to work within the confines of an algebra definition. The class hierarchy should not be what defines the algebra IMHO. You need to define an algebra first first eventually using the defaults. But the mere instantiation of one of the elements, functions or symbol outside of a defined algebra should not be allowed.

I hope this will become clearer when I push some exploration branch.

pombredanne commented 8 years ago

A side note on the singletons for TRUE and FALSE: I do not like singletons here... but in anycase these would never be global singletons. At best these would be each a single instance reused within the definition of an algebra.

pombredanne commented 8 years ago

See #41 for my explorations.

pombredanne commented 8 years ago

@Kronuz @bastikr This is a wip and I am not entirely happy about the class hierarchy that I did not yet change.

pombredanne commented 8 years ago

Note that Expression should be called now BooleanAlgebra and a new base class for terms should be used instead of Expression. I see no reason except convenience to have both Symbol, BaseElement and Functions derive from the same superclass except that they some kind of expression term. But their state is vastly different. Neither Symbol nor BaseElements should have args for instance.

pombredanne commented 8 years ago

Fixed with the merging of #41