sympy
sympy copied to clipboard
sympy
make as_base_exp literal, like fraction
as_base_exp no longer does any introspection, it just returns the literal arguments of the Pow, so the return values of S.Half**x and 1/2**x are no longer the same.
>>> [i.as_base_exp() for i in (S.Half**x, 1/2**x)]
[(1/2, x), (2, -x)]
Release Notes
- core
- as_base_exp output is now literally (self.base, self.exp)
:white_check_mark:
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.
`as_base_exp` no longer does any introspection, it just returns the literal arguments of the `Pow`, so the return values of `S.Half**x` and `1/2**x` are no longer the same.
```python
>>> [i.as_base_exp() for i in (S.Half**x, 1/2**x)]
[(1/2, x), (2, -x)]
```
#### Release Notes
<!-- Write the release notes for this release below between the BEGIN and END
statements. The basic format is a bulleted list with the name of the subpackage
and the release note for this PR. For example:
* solvers
* Added a new solver for logarithmic equations.
* functions
* Fixed a bug with log of integers. Formerly, `log(-x)` incorrectly gave `-log(x)`.
* physics.units
* Corrected a semantical error in the conversion between volt and statvolt which
reported the volt as being larger than the statvolt.
or if no release note(s) should be included use:
NO ENTRY
See https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more
information on how to write release notes. The bot will check your release
notes automatically to see if they are formatted correctly. -->
<!-- BEGIN RELEASE NOTES -->
* core
* as_base_exp output is now literally (self.base, self.exp)
<!-- END RELEASE NOTES -->
This is a good experiment. If we were to go this way then I would also remove Mul.as_base_exp but perhaps better to experiment in separate PRs.
Please check failures to see if this is worth pursuing.
Whether this is worth pursing is a different question from whether it is worth investigating (or maybe that is what you meant by "pursuing" anyway). I am not sure that anything needs to be change in the first place but none of the failures seen in the tests concerns me much.
My default position is always that we should change nothing unless there is a clear reason for making a change.
Wouldn't the equivalent of as_base_exp be as_numer_denom, which does do rewriting? Having something that works structurally is a good idea, but I don't know if it should be as_base_exp or a separate function.
Wouldn't the equivalent of
as_base_expbeas_numer_denom
These are very different types of operation. A problem with using classes and methods for these things is that it makes it difficult to see in one place what any function does.
The previous behavior of as_base_exp was more as_numer_denom like; now it is like fraction with the base and exp attributes being like numer and denom. powsimp(eq).as_base_exp() could be more like as_numer_denom().
The previous behavior of
as_base_exp
It is not clear what version you refer to by "previous" here. The behaviour of as_base_exp was unchanged for a long time and then recently changed by you and then I reverted that change. Here is how the function has changed over time.
In SymPy 0.5.0 it was like this:
def as_base_exp(expr):
if isinstance(expr, Pow):
base, exp = self.args
if base.is_Rational and base.p == 1:
base, exp = 1/base, -exp
elif expr == I:
base, exp = -S.One, S.Half
else:
base, exp = expr, S.One
return base, exp
By SymPy 1.0 it was changed to:
def as_base_exp(expr):
if isinstance(expr, Pow):
base, exp = self.args
if base.is_Rational and base.p == 1:
base, exp = 1/base, -exp
elif isinstance(expr, exp):
base, exp = E, S.One
elif expr == I:
base, exp = -S.One, S.Half
elif isinstance(expr, Mul):
base, exp = as_base_exp_mul(expr)
else:
base, exp = expr, S.One
return base, exp
def as_base_exp_mul(expr):
e1 = None
bases = []
nc = 0
for m in expr.args:
b, e = m.as_base_exp()
if not b.is_commutative:
nc += 1
if e1 is None:
e1 = e
elif e != e1 or nc > 1:
return self, S.One
bases.append(b)
return Mul(*bases), e1
So an extra case for exp(x) was added but also an additional handler for Mul was added (by Chris). That additional handler for Mul is significantly more complicated then the previous cases.
Generally the purpose of as_bae_exp is really just to be able to get the base and exponent of a Pow in a context where the expression might be a Pow but might not and then we have a few extra cases where something that is not a Pow also implements the method. Those extra cases are:
In [3]: I.as_base_exp()
Out[3]: (-1, 1/2)
In [4]: (Rational(1, 3)**x).as_base_exp()
Out[4]: (3, -x)
In [5]: exp(x).as_base_exp()
Out[5]: (ℯ, x)
Most of these cases satisfy an invariant which is that if you pass the result back to Pow then you recover the original expression:
In [15]: exprs = [I, Rational(1,3)**x, exp(x), x**2]
In [16]: for e in exprs:
...: print(Pow(*e.as_base_exp()) == e)
...:
True
False
True
True
The case that fails here is:
In [24]: e = Rational(1,3)**x
In [25]: e
Out[25]:
-x
3
In [26]: e.as_base_exp()
Out[26]: (3, -x)
In [27]: Pow(*e.as_base_exp())
Out[27]:
-x
3
In [28]: Pow(*e.as_base_exp()) == e
Out[28]: False
Confusingly these two expressions display the same in the pretty representation when they are actually different Pows:
In [29]: e.args
Out[29]: (1/3, x)
In [30]: Pow(*e.as_base_exp()).args
Out[30]: (3, -x)
Maybe it would be better for one of these to evaluate to the other. At least they should not display the same if they are not the same. I think that the reason they display the same is because of the use of fraction in the printing code and indirectly the use of as_base_exp. Again the printing code is doing too much here. It does not make sense to use as_base_exp in the printing code because the printing code should be handling these special cases like I and exp(x), etc. The as_base_exp method is supposed to be for code that wants to accept a generic expression but handle the case of a power of it is a Pow or very "close" to a Pow. Already I think the fact that Rational(1,3).as_base_exp() does not recover the correct args for a Pow shows that the extra handling in the Pow case was questionable.
The extra case for Mul is the other one that is trying to rewrite the expression and it basically does this:
In [13]: (x**z*y**z).as_base_exp()
Out[13]: (x⋅y, z)
In [14]: (x*y)**z == x**z*y**z
Out[14]: False
This is buggy which is why I have proposed to change it in gh-26885. There I have restricted it to integer exponents. In general though I think it would be better just to remove the Mul.as_base_exp method. It already goes against the grain of what as_base_exp was previously supposed to do by complicating what was otherwise an extremely simple and predictable routine.
The as_base_exp method was then unchanged for a long time until recently when Chris changed the Pow handler to invert generic rational numbers. Previously it only did so if the numerator was 1 like:
((1/2)**x).as_base_exp() -> (2, -x)
As noted above this is already a questionable manipulation. Chris' more recent change extended this to all rationals if the numerator was less than the denominator (in gh-23921 but more recently reverted in gh-26884):
def as_base_exp(expr):
if isinstance(expr, Pow):
base, exp = self.args
if base.is_Rational and base.p < base.q and base.p > 0:
base, exp = 1/base, -exp
elif isinstance(expr, exp):
base, exp = E, S.One
elif expr == I:
base, exp = -S.One, S.Half
elif isinstance(expr, Mul):
base, exp = as_base_exp_mul(expr)
else:
base, exp = expr, S.One
return base, exp
Then we had:
In [1]: (Rational(2,3)**x).as_base_exp()
Out[1]: (3/2, -x)
In [2]: (Rational(3,2)**x).as_base_exp()
Out[2]: (3/2, x)
The stated purpose of this was to make the bases canonical but there has never been any previous suggestion that as_base_exp should make the bases canonical. Really its purpose is that you can write something like:
b, e = expr.as_base_exp()
without first needing to check expr.is_Pow. This is useful in particular because in many cases a Pow will transform into something that is not a Pow like x**1 -> x, sqrt(-1) -> I, E**x -> exp(x). In fact I think that this is probably a good definition for what as_base_exp should do. It is effectively Pow.make_args by analogy with Add.make_args. It gives you the args so that Pow(*e.as_base_exp()) evaluates to e.
The change in this PR removes the complication in the Pow.as_base_exp handler including the questionable cases I've highlighted above so that we have:
def as_base_exp(expr):
if isinstance(expr, Pow):
base, exp = self.args
elif isinstance(expr, exp):
base, exp = E, S.One
elif expr == I:
base, exp = -S.One, S.Half
elif isinstance(expr, Mul):
base, exp = as_base_exp_mul(expr)
else:
base, exp = expr, S.One
return base, exp
Having written out all of the above I am convinced that this is a good change and is really what should be done. We should also make clear in the docs what the actual purpose of as_base_exp is which is to give you the args for Pow that would reconstruct the expression.
I also think that the Mul handler should maybe be completely removed but let's get gh-26885 merged first which at least makes the behaviour mathematically correct. I suspect that removing the special case Mul here altogether would be more disruptive. Note that restricting the exponent to be an integer in the Mul case does at least satisfy the Pow-args invariant e.g.:
In [1]: (x*y)**2
Out[1]:
2 2
x ⋅y
In [2]: ((x*y)**2).as_base_exp()
Out[2]: (x⋅y, 2)
In [3]: Pow(*((x*y)**2).as_base_exp())
Out[3]:
2 2
x ⋅y
The comparisons with as_numer_denom and fraction are not appropriate because those completely deconstruct expressions and as_numer_denom radically transforms the expression recursing down through all Add, Mul and Pow:
In [7]: e = 1/x + y/(x - 3) + (x + 4)**2 + 1/(1 - 1/(1 - z))
In [8]: e
Out[8]:
y 2 1 1
───── + (x + 4) + ───────── + ─
x - 3 1 x
1 - ─────
1 - z
In [9]: e.as_numer_denom()
Out[9]:
⎛ 2 ⎞
⎝-x⋅y⋅z - x⋅z⋅(x - 3)⋅(x + 4) + x⋅(1 - z)⋅(x - 3) - z⋅(x - 3), -x⋅z⋅(x - 3)⎠
The appropriate comparison for as_base_exp is really something like Add.make_args: it gives you the args for an Add even if the expression is not actually an Add.
As an aside I think it is worth noting how much nicer it is to just have an as_base_exp function where you can clearly see all the cases rather than having it implemented in methods that are distributed across many different classes. Also if add_base_exp was a very simple function then it would be clearer that you can easily write an alternative version of it if you have some need for that like:
def as_base_exp_canonical_base(expr):
base, exp = expr.as_base_exp
# Now do manipulations to make the base canonical
if base.is_Rational and ...
Then it is clearer that there is no need to take what was originally a very simple function and make it progressively more complicated over time.
restricting the exponent to be an integer in the Mul case does at least satisfy the Pow-args invariant
It only does so because of auto-expansion of (x*y)**2 -> x**2*y**2.