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.

Sign up for GitHub

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

Jump to bottom

Make count_ops() work with logic operations by creating a binary tree. #7303

Merged
merged 26 commits into from Apr 12, 2014
Merged

Make count_ops() work with logic operations by creating a binary tree. #7303

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
cefd428
Make count_ops() work with logic operations by creating a binary tree…
sahilshekhawat Mar 19, 2014
5389065
Added tests of the logic functions for count_ops() function.
sahilshekhawat Mar 19, 2014
7000d9e
Fixed a test case of Xor(x,y) = 2*AND + OR
sahilshekhawat Mar 19, 2014
3c5ba38
Fixed the test case for Nand(x,y).count_ops(visual=True) = OR and Nor…
sahilshekhawat Mar 19, 2014
07168de
Removed the obsolete test case which was addressing the bug.
sahilshekhawat Mar 19, 2014
25a9686
Included the case of logic function in the "else" case.
sahilshekhawat Mar 19, 2014
d5abbd7
removed the XXX comment to report bug.
sahilshekhawat Mar 19, 2014
366f864
Removed the case for logic functions because it was contradicting wit…
sahilshekhawat Mar 19, 2014
00604b1
Final commit, added the logic function to be counted in count_ops()
sahilshekhawat Mar 19, 2014
d374ed5
remove the else condition and added the is.Boolean condition to gener…
sahilshekhawat Mar 22, 2014
d277512
Removed redundant tests count_ops(Basic()) = S.zero
sahilshekhawat Mar 22, 2014
dc6cf28
Removed trailing whitespaces, will always check them first.
sahilshekhawat Mar 22, 2014
ee126ac
Added the cases to handle the exception of True and False, who were b…
sahilshekhawat Mar 23, 2014
b49c02c
Fixed the bug.
sahilshekhawat Mar 23, 2014
2f7d1de
Merge branch 'master' of https://github.com/sympy/sympy
sahilshekhawat Mar 23, 2014
e1c8ea4
Fix the bug due to which `Basic()` was counted as an operation and us…
sahilshekhawat Mar 28, 2014
7adde66
Added more tests which also include some complex ones.
sahilshekhawat Mar 28, 2014
cc74cd8
removed extra spaces
sahilshekhawat Mar 28, 2014
80fd37a
removed ``true`` and ``false`` and used ``not a.args is ()`` instead …
sahilshekhawat Mar 29, 2014
912c0fd
Replace "is" with "==" to check the conditions.
sahilshekhawat Mar 29, 2014
9549f0b
replaced 'is' with '=='
sahilshekhawat Apr 4, 2014
37ba49b
Added tests to include cases of logic functions given less arguments …
sahilshekhawat Apr 5, 2014
4fbde7b
removed Basic and Tuple check
sahilshekhawat Apr 10, 2014
9771a86
Added more test related to Basic()
sahilshekhawat Apr 10, 2014
5d3950d
Added logical test for TUPLE and removed duplicate tests
sahilshekhawat Apr 11, 2014
189c659
removed some tests which were showing bugs in logical functions.
sahilshekhawat Apr 11, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 17 additions & 1 deletion sympy/core/function.py
View file
Open in desktop
Expand Up @@ -2326,7 +2326,23 @@ def count_ops(expr, visual=False):
else: # it's Basic not isinstance(expr, Expr):
if not isinstance(expr, Basic):
raise TypeError("Invalid type of expr")
ops = [count_ops(a, visual=visual) for a in expr.args]
else:
true = sympify(True)
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.

Why these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because without these, True and False were being treated as logic functions. so i had to add their cases and also i was not able to import true and false from logic.boolalg thus i used sympify instead

//without
In [1]: count_ops(True)
Out [1]: 1
//with
In [2]: count_ops(True)
Out [2]: 0

Copy link
Member

Choose a reason for hiding this comment

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

count_ops should just not consider any atomic object (no .args) as having 0 operations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @asmeurer, i have corrected it. I know it was a silly mistake but I am still learning.. please don't mind.

ops = []
args = [expr]
while args:
a = args.pop()
o = C.Symbol(a.func.__name__.upper())
if (not expr is true and not expr is false):
ops.append(o*(len(a.args)-1))
Copy link
Member

Choose a reason for hiding this comment

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

You don't even need the if blocks. This will be a noop if the args are empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think i do, as the args is just a list of expressions for which we are checking ops and the if condition is checking if the expressions which is poped from args have any arguments or not without it all the expressions whether they have any args or not will be appended to args.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I am off by one. You are right.

But why do you need to check against Basic and Tuple. I don't like that. This if should just be if a.args.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sadly i think i have to include it...other wise Basic() will give BASIC as one operation and tuple is for a special case when there are brackets in one of the arguments itself e.g. And((x,y),y).count_ops()...I think i should add this.

Copy link
Member

Choose a reason for hiding this comment

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

Add((x, y), y) doesn't even make sense.

But I think this is right. In this case, Basic and Tuple are the operations. Basic() with no args should give nothing because it has no args, but that should be handled generally in the code (and will be handled if you do if a.args).

Copy link
Member

Choose a reason for hiding this comment

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

The lines

+                    not a.__class__.__name__ == 'Basic' and
+                    not a.__class__.__name__ == 'Tuple'):
+                    o = C.Symbol(a.func.__name__.upper())

Look really hackish to me. We should not use the __class__ attributes unless we absolutely have to. There is no way to implement this differently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am going to remove not a.__class__.__name__ == 'Basic' but Tuple is the only possible exception..What do you suggest? should we use another method?
Thanks

Copy link
Member

Choose a reason for hiding this comment

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

I think every Basic subclass should be counted as an operation, if it has args.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, i will remove !not a.__class__.__name__ == 'Basic' and !
Anything else which must change? should i remove not a.__class__.__name__ == 'Tuple'):? Please suggest me more changes so that it can be merged soon!
Thank you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@asmeurer I wish to put one thing in you notice that Expr(x).count_ops() == 0 which is already written..but still i am including BASIC as an operation.

args_len = len(a.args)
aargs = list(a.args)
for i in range(args_len):
if (not aargs[i].is_Symbol and
not aargs[i] is true and
not aargs[i] is false):
args.append(aargs[i])

if not ops:
if visual:
Expand Down
27 changes: 21 additions & 6 deletions sympy/core/tests/test_count_ops.py
View file
Open in desktop
@@ -1,5 +1,5 @@
from sympy import symbols, sin, exp, cos, Derivative, Integral, Basic, \
count_ops, S, And, I, pi, Eq
count_ops, S, And, I, pi, Eq, Or, Not, Xor ,Nand ,Nor, Implies,Equivalent, ITE

x, y, z = symbols('x,y,z')

Expand All @@ -14,12 +14,22 @@ def count(val):
assert count(x + y*x + 2*y) == 4
assert count({x + y: x}) == 1
assert count({x + y: S(2) + x}) is not S.One

assert count(Or(x,y)) == 1
assert count(And(x,y)) == 1
assert count(Not(x)) == 0
assert count(Nor(x,y)) == 1
assert count(Nand(x,y)) == 1
assert count(Xor(x,y)) == 3
assert count(Implies(x,y)) == 1
assert count(Equivalent(x,y)) == 1
assert count(ITE(x,y,z)) == 3

def test_count_ops_visual():
ADD, MUL, POW, SIN, COS, EXP, AND, D, G = symbols(
'Add Mul Pow sin cos exp And Derivative Integral'.upper())
DIV, SUB, NEG = symbols('DIV SUB NEG')
OR, AND, IMPLIES, EQUIVALENT = symbols(
'Or And Implies Equivalent'.upper())

def count(val):
return count_ops(val, visual=True)
Expand Down Expand Up @@ -69,7 +79,6 @@ def count(val):

assert count(Derivative(x, x)) == D
assert count(Integral(x, x) + 2*x/(1 + x)) == G + DIV + MUL + 2*ADD
assert count(Basic()) is S.Zero

assert count({x + 1: sin(x)}) == ADD + SIN
assert count([x + 1, sin(x) + y, None]) == ADD + SIN + ADD
Expand All @@ -78,7 +87,13 @@ def count(val):
assert count([x + 1, sin(x)*y, None]) == SIN + ADD + MUL
assert count([]) is S.Zero

# XXX: These are a bit surprising, only Expr-compatible ops are counted.
assert count(And(x, y, z)) == 0
assert count(Basic(x, x + y)) == ADD
assert count(Or(x,y)) == OR
assert count(And(x,y)) == AND
assert count(Nor(x,y)) == AND
assert count(Nand(x,y)) == OR
assert count(Xor(x,y)) == 2*AND + OR
assert count(Implies(x,y)) == IMPLIES
assert count(Equivalent(x,y)) == EQUIVALENT
assert count(ITE(x,y,z)) == 2*AND + OR

assert count(Eq(x + y, S(2))) == ADD