fix(core): restrict the 2-arg Mul to -1 #27200
Conversation
|
✅ Hi, I am the SymPy bot. I'm here to help you write a release notes entry. Please read the guide on how to write release notes. Your release notes are in good order. Here is what the release notes will look like:
This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.14. Click here to see the pull request description that was parsed. |
Since 3e835b5 from sympygh-716 there is the following behaviour: >>> 2*(x + y) 2*x + 2*y Specifically if there is a Mul with 2 args and one arg is a Rational and the other an Add then the Rational is distributed into the Add. More generally if there is a Mul and the arguments collapse to a number and an Add e.g. Mul(2, 3.0, x + y) then the coefficient is distributed into the Add. There are various problems with this behaviour such as the fact that it breaks associativity (a*b)*c != a*(b*c). There have been previous attempts to remove this e.g. sympygh-17242 but it is not easy because so much code now depends on it. This commit attempts a more limited change in which the 2-arg Mul is mostly removed but kept for one important special case: if we have Mul(-1, Add(...)) then the -1 will be distributed into the Add. This special case is important because it arises when subtracting to Adds e.g.: >>> x + y - (x + y) 0 It could perhaps be possible to special case negation and subtraction somehow so that negating the terms of an Add happens specifically in subtraction rather than more generally in a Mul but here we just preserve the 2-arg behaviour for -1 specifically as a more limited change to existing behaviour. Keeping this particular special case of the 2-arg Mul dramatically reduces the number of test failures seen in the test suite comared to removing automatic distribution altogether.
e942284
to
9e9c4d9
Compare
Interestingly Maxima has this behaviour: It seems that there is a special case for SymEngine does not do this at all e.g.: In [1]: import symengine
In [2]: x, y = symengine.symbols('x, y')
In [3]: x + y - (x + y)
Out[3]: x + y - (x + y)It is hard to say what is actually best here but from running parts of the test suite and studying the failures there are far bigger and deeper failures that come from matching the SymEngine behaviour than from matching the Maxima behaviour here. When restricting distribution to |
I think in so far as possible though it would be better not to update the tests but to try to find where in the code the automatic distribution was happening and add it back in explicitly so that the output comes in the same form. |
|
Looks like trigsimp doesn't work: In [11]: trigsimp(cos(x)**2 - sin(x)**2)
Out[11]:
2
1 - 2⋅sin (x)On master it is: In [1]: trigsimp(cos(x)**2 - sin(x)**2)
Out[1]: cos(2⋅x)Somewhere in trigsimp there must be an assumption that something distributes in order to make this simplification work. |
|
This fails in the PR: In [1]: cot(pi/24).radsimp() # master
Out[1]: √2 + √3 + 2 + √6
In [1]: cot(pi/24).radsimp() # PR
Out[1]:
2
(√2 + √6) ⋅(-√2 + 1 + √6)
─────────────────────────
4 |
|
It seems powsimp needs a bit of help: In [1]: powsimp(4**k*4**(-k - 1), combine='exp') # master
Out[1]: 1/4
In [1]: powsimp(4**k*4**(-k - 1), combine='exp') # PR
Out[1]:
2⋅k + 2⋅(-k - 1)
2 |
|
Mod needs some help: In [1]: (12*x + 18*y) % (3*x) # master
Out[1]: 3⋅(6⋅y mod x)
In [1]: (12*x + 18*y) % (3*x) # PR
Out[1]: 3⋅(2⋅(2⋅x + 3⋅y) mod x) |
|
This should probably distribute: In [2]: sqrt(-3 - 4*I) # master
Out[2]: 1 - 2⋅ⅈ
In [1]: sqrt(-3 - 4*I) # PR
Out[1]: 2⋅(1/2 - ⅈ)I think that the intention here is to have a canonical form of Gaussian rational if possible so it should be expanded. |
|
Also this difference: In [1]: sqrt((3 + 4*I)/4) # master
Out[1]:
ⅈ
1 + ─
2
In [1]: sqrt((3 + 4*I)/4) # PR
Out[1]:
2 + ⅈ
─────
2This one is handled in |
Since 3e835b5 from gh-716 there is the following behaviour:
Specifically if there is a Mul with 2 args and one arg is a Rational and the other an Add then the Rational is distributed into the Add. More generally if there is a Mul and the arguments collapse to a number and an Add e.g.
Mul(2, 3.0, x + y)then the coefficient is distributed into the Add.There are various problems with this behaviour such as the fact that it breaks associativity
(a*b)*c != a*(b*c). Previous discussions have agreed that this should be removed (e.g. gh-4596). There have been previous attempts to remove this e.g. gh-17242 but it is not easy because so much code now depends on it.This commit attempts a more limited change in which the 2-arg Mul is mostly removed but kept for one important special case: if we have
Mul(-1, Add(...))then the -1 will be distributed into the Add. This special case is important because it arises when subtracting to Adds e.g.:It could perhaps be possible to special case negation and subtraction somehow so that negating the terms of an Add happens specifically in subtraction rather than more generally in a Mul but here we just preserve the 2-arg behaviour for -1 specifically as a more limited change to existing behaviour. Keeping this particular special case of the 2-arg Mul dramatically reduces the number of test failures seen in the test suite compared to removing automatic distribution altogether.
References to other Issues or PRs
Brief description of what is fixed or changed
Other comments
Release Notes
2*(x + y)remains as2*(x + y)rather than distributing as2*x + 2*y. Distribution is now limited to the special case in which the multiplier is-1so-(x + y)still transforms to-x - ybut any other coefficient will not be distributed.