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

sahilshekhawat · 2014-03-19T08:41:32Z

Bug was that Logic functions are not the instance of sympy.core.expr.Expr due to which logic operation were actually skipping to "else" case.

Fixes #7010

…. Bug was that Logic functions are not the instance of sympy.core.expr.Expr due to which logic operation were actually skipping to "else" case. Fixes #7010

Zamrath · 2014-03-19T09:01:20Z

sympy/core/function.py

+        args = [expr]
+        while args:
+            a = args.pop()
+            o = C.Symbol(a.func.__name__.upper())


I may not wonder if args.pop() will return whole the expr you import regardless how larger that is. Something like a = expr.args() would do the trick.

Its for the condition when two or more args of the expr are also logic functions( something like binary tree), in that case they will be appended to args and args.pop() will run the loop for each of them.
yes! you are right, initially it will return the whole expression regardless of how large it is.

asmeurer · 2014-03-19T15:50:37Z

Please add tests.

sahilshekhawat · 2014-03-19T15:55:51Z

I have done all the test locally on my system. but i don't know how to add tests here (Travis one's)

debugger22 · 2014-03-19T16:05:47Z

@sahilshekhawat There's a file called .travis.yml in the repository. It contains information related to the test files. You just add tests for the particular function you added in the corresponding test file and the github will take care of the rest.

sahilshekhawat · 2014-03-19T16:07:49Z

But i have not added any functions i have just added some lines to an existing function.

skirpichev · 2014-03-19T16:10:55Z

At least, you should add tests from the referenced issue.

sahilshekhawat · 2014-03-19T16:16:29Z

I got it! Thanks

…(x,y).count_ops(visual=True) = AND

sahilshekhawat · 2014-03-19T17:53:29Z

sympy/core/tests/test_count_ops.py

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


This test is causing an error should i remove it. because the bug is now fixed

okay, Thanks 👍

…h every other function's ops which is Basic but not isinstance(expr,Expr)

asmeurer · 2014-03-19T23:34:01Z

I mean, your algorithm is general. It can work on anything, not just Boolean. So you should replace elif expr.is_Boolean with just else (and also get rid of the if aargs[i].is_Boolean line below).

sahilshekhawat · 2014-03-20T01:37:18Z

My algorithm is same as that used above in the case of is_Add for example, and for else there is already an condition see ops = [count_ops(a, visual=visual) for a in expr.args]
which handles everything else.
and about removing if aargs[i].is_Boolean, then if one of the args is just a constant or symbol, how can we distinguish it from any function. can i use is_Function?

asmeurer · 2014-03-20T19:14:00Z

But your algorithm actually counts all operations. I'd rather have that, even for Basic ops.

sahilshekhawat · 2014-03-20T19:16:23Z

so to separate the symbols from the Basic ops should i use isinstance(aargs, Basic) or aargs.is_Function?

sahilshekhawat · 2014-03-21T16:15:55Z

isinstance(aargs, Basic) will not work and i think is_Function is just a subset of Basic. and i did what you said but removing aargs.isBoolean, is giving wrong answers.
I have used if not aargs.is_Symbol: but it is failing the test because count_ops(Basic()) expects S.zero which was in the else conditions..i think that either the current solution is correct or we need to change test case. any advice?

sahilshekhawat · 2014-03-22T16:09:08Z

@skirpichev can you please help me?

…al Basic instances.

sahilshekhawat · 2014-03-22T17:24:40Z

Sorry for the extra commit for trailing white spaces..i forgot to run ./bin/test quality before commiting. Will take care of that from the next time.

skirpichev · 2014-03-22T17:25:38Z

@skirpichev can you please help me?

I'm slightly off this discussion. What help do you need?

sahilshekhawat · 2014-03-22T17:27:01Z

Sorry for that mention, Aaron just cleared my doubt on gitter. I think it will work for any equation now, even those who are the instance of Basic()
Thanks

asmeurer · 2014-03-26T18:37:20Z

sympy/core/tests/test_count_ops.py

@@ -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


Don't remove the test. Replace it with the new value. And add some more tests for more complicated expressions like Basic(Basic(), Basic()) or Basic(x + y) and so on.

Okay! I am adding more complex tests for other logic functions and Basic() too

…ed a better algo for parsing args.

sahilshekhawat · 2014-03-28T18:05:38Z

@asmeurer I have done just as you suggested. Please review it!

…as suggested by aaron.

sahilshekhawat · 2014-03-29T04:20:46Z

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

sahilshekhawat · 2014-03-29T15:39:10Z

@asmeurer on my system the same code count_ops([x, sin(x), None, True, x + 2], visual=False) is running perfectly...but travis is showing an error. Can you please help me on what could be the possible cause.??

sahilshekhawat · 2014-03-30T09:26:36Z

Thank you @asmeurer for pointing that mistake, now as it has passed all the tests, can we merge it now?

asmeurer · 2014-04-02T14:43:01Z

sympy/core/tests/test_count_ops.py

+    assert count(Implies(x,y)) == IMPLIES
+    assert count(Equivalent(x,y)) == EQUIVALENT
+    assert count(ITE(x,y,z)) == 2*AND + OR
+    assert count(And((((x,y+z))))) == ADD


This doesn't even use And

This is because the simpify(And((((x,y+z))))) == (x, y+z) and thats why it is returning ADD.
This is due to the fact that Add(x) == x when there is only one argument instead of two in the Add()

This is actually a special case thats why i have included it.

You can at least remove the redundant parentheses.

You should construct expressions that don't make sense, like And of a tuple.

ya, it doesn't make sense to include so many parenthesis. :)
Okay, i am writing them..Thanks

sahilshekhawat · 2014-04-04T15:59:04Z

Is everything okay now?

…than require

sahilshekhawat · 2014-04-06T02:22:51Z

@asmeurer please have a look, i have added the tests.

asmeurer · 2014-04-11T17:02:26Z

sympy/core/tests/test_count_ops.py

+    assert count(Basic()) == 0
+    assert count(Basic(Basic(),Basic(x,x+y))) == ADD + 2*BASIC
+    assert count(Basic(x, x + y)) == ADD + BASIC
+    assert count(Basic(Basic(), Basic(x,x+y))) ==  ADD + 2*BASIC


You have this test twice.

okay, i am removing this.

asmeurer · 2014-04-11T19:05:54Z

sympy/core/tests/test_count_ops.py

+
+    # The test given below checks the results which comes when logical
+    # functions are given less number of arguments than required and
+    # don't raises an exception. They don't make much sense.


I would just remove these tests. They are all either things that don't make sense or operations that return a different object than what is started with. I think these tests are just confusing, especially if any of these things are changed.

okay! i am commiting after removing these.

asmeurer · 2014-04-11T19:06:12Z

I have just that one comment. The rest looks good.

sahilshekhawat · 2014-04-11T19:10:47Z

I have removed those tests...
Do we have to wait for Travis to finish the tests?

asmeurer · 2014-04-11T19:22:12Z

Yes.

sahilshekhawat · 2014-04-11T19:23:12Z

Thank you Aaron, for your help.

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

Make count_ops() work with logic operations by creating a binary tree…

cefd428

…. Bug was that Logic functions are not the instance of sympy.core.expr.Expr due to which logic operation were actually skipping to "else" case. Fixes #7010

Zamrath reviewed Mar 19, 2014
View reviewed changes

sahilshekhawat added 3 commits March 19, 2014 22:06

Added tests of the logic functions for count_ops() function.

5389065

Fixed a test case of Xor(x,y) = 2*AND + OR

7000d9e

Fixed the test case for Nand(x,y).count_ops(visual=True) = OR and Nor…

3c5ba38

…(x,y).count_ops(visual=True) = AND

sahilshekhawat reviewed Mar 19, 2014
View reviewed changes

sahilshekhawat added 5 commits March 19, 2014 23:32

Removed the obsolete test case which was addressing the bug.

07168de

Included the case of logic function in the "else" case.

25a9686

removed the XXX comment to report bug.

d5abbd7

Removed the case for logic functions because it was contradicting wit…

366f864

…h every other function's ops which is Basic but not isinstance(expr,Expr)

Final commit, added the logic function to be counted in count_ops()

00604b1

sahilshekhawat added 3 commits March 22, 2014 22:46

remove the else condition and added the is.Boolean condition to gener…

d374ed5

…al Basic instances.

Removed redundant tests count_ops(Basic()) = S.zero

d277512

Removed trailing whitespaces, will always check them first.

dc6cf28

asmeurer reviewed Mar 26, 2014
View reviewed changes

sahilshekhawat added 3 commits March 28, 2014 21:16

Fix the bug due to which Basic() was counted as an operation and us…

e1c8ea4

…ed a better algo for parsing args.

Added more tests which also include some complex ones.

7adde66

removed extra spaces

cc74cd8

removed true and false and used not a.args is () instead …

80fd37a

…as suggested by aaron.

Replace "is" with "==" to check the conditions.

912c0fd

asmeurer reviewed Apr 2, 2014
View reviewed changes

replaced 'is' with '=='

9549f0b

Added tests to include cases of logic functions given less arguments …

37ba49b

…than require

sahilshekhawat added 2 commits April 10, 2014 22:16

removed Basic and Tuple check

4fbde7b

Added more test related to Basic()

9771a86

asmeurer reviewed Apr 11, 2014
View reviewed changes

Added logical test for TUPLE and removed duplicate tests

5d3950d

asmeurer reviewed Apr 11, 2014
View reviewed changes

removed some tests which were showing bugs in logical functions.

189c659

asmeurer added a commit that referenced this pull request Apr 12, 2014

Merge pull request #7303 from sahilshekhawat/master

f20add3

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

asmeurer merged commit f20add3 into sympy:master Apr 12, 2014

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

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

Conversation

sahilshekhawat commented Mar 19, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asmeurer commented Mar 19, 2014

sahilshekhawat commented Mar 19, 2014

debugger22 commented Mar 19, 2014

sahilshekhawat commented Mar 19, 2014

skirpichev commented Mar 19, 2014

sahilshekhawat commented Mar 19, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asmeurer commented Mar 19, 2014

sahilshekhawat commented Mar 20, 2014

asmeurer commented Mar 20, 2014

sahilshekhawat commented Mar 20, 2014

sahilshekhawat commented Mar 21, 2014

sahilshekhawat commented Mar 22, 2014

sahilshekhawat commented Mar 22, 2014

skirpichev commented Mar 22, 2014

sahilshekhawat commented Mar 22, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahilshekhawat commented Mar 28, 2014

sahilshekhawat commented Mar 29, 2014

sahilshekhawat commented Mar 29, 2014

sahilshekhawat commented Mar 30, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahilshekhawat commented Apr 4, 2014

sahilshekhawat commented Apr 6, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asmeurer commented Apr 11, 2014

sahilshekhawat commented Apr 11, 2014

asmeurer commented Apr 11, 2014

sahilshekhawat commented Apr 11, 2014