suppressed evaluation for Add and Mul

[chris...@kwyk.fr]

Hello everyone,

It is my first post and I hope it is the right place to send my question. First I would like to thank all the developers who created this great library. We use it in my company but we encountered a very strange behavior. I wanted to do a PR to correct it but the tests show that this may be voluntary behavior.

The problem : even with "evaluate=False" argument, Mul and Add remove identity elements from its arguments. We can see this behavior in tests : 

https://github.com/sympy/sympy/blob/master/sympy/core/tests/test_arit.py#L1366

def test_suppressed_evaluation():
    a = Add(0, 3, 2, evaluate=False)
    b = Mul(1, 3, 2, evaluate=False)
    c = Pow(3, 2, evaluate=False)
    assert a != 6
    assert a.func is Add
    assert a.args == (3, 2)
    assert b != 6
    assert b.func is Mul
    assert b.args == (3, 2)
    assert c != 9
    assert c.func is Pow
    assert c.args == (3, 2)

Removing arguments is already a kind of evaluation. For me, we should have :

def test_suppressed_evaluation():
    a = Add(0, 3, 2, evaluate=False)
    b = Mul(1, 3, 2, evaluate=False)
    # ...
    assert a.args == (0, 3, 2)
    # ...
    assert b.args == (1, 3, 2)
    # ...

and do not remove any arguments.

I can do a PR to change this behavior (and update tests), It seems to me that this can be corrected by moving the lines that remove identities elements right after the "evaluate" processing : move the line https://github.com/sympy/sympy/blob/master/sympy/core/operations.py#L31 after the if on line 33.

I prefer to ask the question here before because the presence of the tests can make believe that this behavior is voluntary.

Christophe Gabard

[Aaron Meurer]

Are there any other tests that fail if you make the change? 

I would look into the git history to see what changes added those tests if they made any justification for it. It seems to me that evaluate=False shouldn't be removing identities, but maybe there was a reason for it.

Aaron Meurer

--
You received this message because you are subscribed to the Google Groups "sympy" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sympy+unsubscribe@googlegroups.com.
To post to this group, send email to sy...@googlegroups.com.
Visit this group at https://groups.google.com/group/sympy.
To view this discussion on the web visit https://groups.google.com/d/msgid/sympy/2b59c125-f424-459b-b5a1-f220e5ef7db2%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[chris...@kwyk.fr]

Hum, tests are currently running on my computer and some to fail.

Some only need updates, like sympy/core/tests/test_evaluate.py which explicitly refer to evaluate = False current behavior. Some other tests seem to fail without apparent reasons. I will investigate this before doing a PR.

Thank you for your reply,

Christophe Gabard

To unsubscribe from this group and stop receiving emails from it, send an email to sympy+un...@googlegroups.com.

[chris...@kwyk.fr]

I made a commit and add some comments about the changes : https://github.com/cgabard/sympy/commit/985e8e8251303aeed2763a2956a95f7d0f8f5ac2

There are several changes in addition to those already mentioned :

 - Some tests in sympy/core/tests/test_evaluate.py rely on `evaluate=False` needs to be updated,

 - `flatten` in sympy/core/mul.py  and `fraction` in sympy/simplify/radsimp.py use `evaluate=False` and rely on the previous behavior.

 - test about kernS : I am not sure what the purpose of this function is, and I do not know what the actual behavior is.

All tests are green on my computer with these modifications.

[Aaron Meurer]

Regarding kernS, see the docstring of sympify(). It is related to preventing automatic simplification due to distribution of constants. I don't think it's supposed to prevent evaluation of constants, so your commit looks like a regression in it. 

It may be helpful here to open a work in progress pull request from your change, to make it easier to comment on it.

Aaron Meurer

To unsubscribe from this group and stop receiving emails from it, send an email to sympy+unsubscribe@googlegroups.com.

To post to this group, send email to sy...@googlegroups.com.
Visit this group at https://groups.google.com/group/sympy.

To view this discussion on the web visit https://groups.google.com/d/msgid/sympy/dd559b97-3d48-4c6f-ab7a-491f4ac82fdb%40googlegroups.com.

[chris...@kwyk.fr]

I have made a WIP PR for this change here : https://github.com/sympy/sympy/pull/13188

Hope all will be ok.