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.

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

Fix for issue #2549; give as_independent more support for non commutative symbols #481

Closed
wants to merge 5 commits into from

Conversation

Projects
None yet
3 participants
@lazovich
Copy link
Contributor

commented Jul 6, 2011

Issue 2549: http://code.google.com/p/sympy/issues/detail?id=2549

There seemed to be a problem where any expression involving the argument to as_independent and a non-commutative symbol was treated as independent even though it wasn't. I added a few lines to check the non-commutative expressions for the argument before adding them to the list of independent expressions. I am not sure if this supports all possible non-commutative expressions, but it does fix the bug I observed in the issue.

@asmeurer

This comment has been minimized.

Copy link
Member

commented Jul 7, 2011

Test results html report: http://pastehtml.com/view/azl2fo64d.html

Automatic review by sympy-bot.

@smichr

This comment has been minimized.

Copy link

commented on f341f2f Jul 7, 2011

One thing to check for is that non-commutatives in the middle of a Mul are not pulled out, e.g. (a*b*n1*n2*n3).as_independent(n2) should not return (a*b*n1*n3, n2) and I think that is what your modification will allow when a Mul is given as self and there are no commutative deps. I need a little time to think about this, but will take a look at it tomorrow.

This comment has been minimized.

Copy link
Owner Author

replied Jul 7, 2011

@smichr: I tried the case you mentioned and got (a_b_n1, n2*n3) for the output, so I think we're ok here. I will add this as a test to be sure it doesn't break in the future.

This comment has been minimized.

Copy link
Owner Author

replied Jul 7, 2011

@smichr: I realize that my previous comment makes sense because the code I added doesn't even affect the case where there is a non-commutative dep passed and the func is a Mul. It will go to the else at line 930 in this case, and that code seems to handle this correctly.

@lazovich

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2011

@smichr: I added a few more tests for the expression that you describe.

@smichr

This comment has been minimized.

Copy link

commented on sympy/core/tests/test_expr.py in 3681245 Jul 7, 2011

btw, this and the preceding test already pass in master.

This comment has been minimized.

Copy link
Owner Author

replied Jul 7, 2011

I just pulled the lastest master and for me, the test on L455 passes in master, but the test on L454 gives (x+n1, x-y) without this fix.

This comment has been minimized.

Copy link

replied Jul 7, 2011

Hmm, I confirmed that as well. Sorry for the false alarm. (Maybe I didn't have n1 being noncommutative.) I think both tests can stay, however.

@smichr

This comment has been minimized.

Copy link
Member

commented Jul 7, 2011

OK, here is the problem: if you have a Mul expression and there are no ndeps then you are going to sift the nc terms and pull something out from the middle:

>>> var('m n o', commutative=0)
(m, n, o)
>>> D=DiracDelta
>>> D(x-n)*D(y-n)*D(z-n)
DiracDelta(x - n)*DiracDelta(y - n)*DiracDelta(z - n)
>>> _.as_independent(y)
(DiracDelta(x - n)*DiracDelta(z - n), DiracDelta(y - n))

Only the DiracDelta(x - n) should have been pulled out. Furthermore, if the y-containing term had been the leftmost term, then it might be nice to get back what was given above. Perhaps there should be an nc flag which returns a 3-member tuple containing left-independent, dependent, right-independent terms so the expression above would have given DiracDelta(x - n), DiracDelta(y - n), DiracDelta(z - n); if there were no z-term then the 2th element would be 1.

@lazovich

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2011

@smichr: The commit I just pushed is a new attempt at fixing both the bug that I discovered and the case that you just mentioned. Basically, I think the fundamental flaw in the old code is that it assumes that the non-commutative terms in nc only have to be checked for ndeps and not for deps as well. This commit fixes that, and it also keeps the old functionality in the else that preserves all terms to the right of a non-commutative term. I added a test for the case you show above as well.

@lazovich

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2011

@smichr: Thanks for your changes! I've merged them in.

@lazovich

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2011

Test results html report: http://pastehtml.com/view/azozc53e4.html

Automatic review by sympy-bot.

@smichr

This comment has been minimized.

Copy link
Member

commented Jul 8, 2011

Thanks. I squashed commits and pushed this.

@smichr smichr closed this Jul 8, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.