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

Inconsistent behavior in evaluation of the absolute value #20266

Open
meganly opened this issue Oct 15, 2020 · 13 comments
Open

Inconsistent behavior in evaluation of the absolute value #20266

meganly opened this issue Oct 15, 2020 · 13 comments

Comments

@meganly
Copy link
Contributor

meganly commented Oct 15, 2020

It seems like there's a bug in how the absolute value is evaluated.

>>> x = symbols('x', real=True)
>>> y = symbols('y', real=True)
>>> Abs(x*y)
Abs(x*y)
>>> Abs(x**2*y)
x**2*Abs(y)
>>> Abs(x**3*y**3)
x**2*y**2*Abs(x)*Abs(y)

I would expectAbs(x*y) to evaluate to Abs(x)*Abs(y).

@oscarbenjamin
Copy link
Contributor

I would prefer it if all the examples remained unevaluated so they could be manipulated like Pow:

In [9]: x, y, z = symbols('x, y, z', positive=True)                                                                                            

In [10]: (x*y)**z                                                                                                                              
Out[10]: 
     z
(xy) 

In [11]: expand((x*y)**z)                                                                                                                      
Out[11]: 
 z  z
x y 

In [12]: powsimp(_)                                                                                                                            
Out[12]: 
     z
(xy) 

@meganly
Copy link
Contributor Author

meganly commented Oct 17, 2020

Agreed. However, expand doesn't work with the absolute value.

>>> x, y = symbols('x, y', real=True)
>>> expand(Abs(x*y))
Abs(x*y)

@oscarbenjamin
Copy link
Contributor

I think it would be good if expand could work with Abs. I don't think that there are any cases where it is not valid that Abs(x*y) == Abs(x)*Abs(y).

@meganly
Copy link
Contributor Author

meganly commented Oct 19, 2020

Agreed. An absolute value on a field is multiplicative so Abs(x*y) == Abs(x)*Abs(y). I feel like the key is to think of the absolute value as Abs(z) := sqrt(z *conjugate(z)).

>>> x, y = symbols('x, y', real=True)
>>> sqrt((x*y)**2)
Abs(x)*Abs(y)

This doesn't seem to work with complex numbers because Sympy doesn't realize that z*conjugate(z) is real.

>>> w, z = symbols('w, z')
>>> expand(sqrt((w*z)*conjugate(w*z)))
sqrt((w*z)*conjugate(w*z))
>>> expand(sqrt((w)*conjugate(w)*z*conjugate(z)))
sqrt((w*z)*conjugate(w*z))

@dhruvmendiratta6
Copy link
Contributor

Are you suggesting adding something like expand_Abs()? I tried a fix for it by changing eval() in Abs, although it gives desired results I don't think changing the default behavior is the way to go in this case.

@meganly
Copy link
Contributor Author

meganly commented Oct 21, 2020

I think expand_power_base, which is one of the hints to expand, should be made smarter to handle absolute values.

@theanshm
Copy link
Contributor

From where can start working on this issue? I tried to search but could not find a starting point. Kindly give me a direction to start with.

@meganly
Copy link
Contributor Author

meganly commented Oct 27, 2020

@theanshm I'm not exactly sure but it looks like there are two issues to fix:

  1. Changing the eval method in Abs to leave the expressions unevaluated. I'm curious what @dhruvmendiratta6 tried.

  2. Getting expand_power_base to evaluate Abs(x*y) as Abs(x)*Abs(y). I would try looking at the code for _eval_expand_power_base or how the absolute value class is implemented.

@theanshm
Copy link
Contributor

While working on this issue I realized that changing the default behaviour is breaking a lot of stuff. Should I just make the output of Abs(x*y) consistent with the other outputs or just go on with changing the default behaviour and modifying the expand function?

@oscarbenjamin
Copy link
Contributor

I'm not sure exactly what you are asking. To be clear I don't expect that it would be easy to make these changes.

@theanshm
Copy link
Contributor

theanshm commented Oct 30, 2020

Actually while working on the above mentioned changes, I noticed that it broke a lot of other tests. So I was just confirming if changing the way that Abs works is the right way to go or should I just make it so that,

In [1]: Abs(x*y)
Out[1]: │x│⋅│y│

which makes it consistent with the other Abs outputs such as:

In [7]: Abs(x**3*y**3)
         2  2        
Out[7]: x ⋅y ⋅│x│⋅│y│

@meganly
Copy link
Contributor Author

meganly commented Nov 1, 2020

Changing how Abs works so we have

>>> x, y = symbols('x, y' real=True)
>>> Abs(x*y)
Abs(x*y)
>>> Abs(x**3*y**3)
Abs(x**3*y**3)

is the right way to go to fix the first issue. That will break a lot of tests which is probably why @oscarbenjamin doesn't expect it to be easy to make these changes.

@theanshm
Copy link
Contributor

theanshm commented Nov 1, 2020

Ok, got it. Thanks for the clarification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants