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

wrong result integrating sin(a*(x+pi))**2 #26566

Open
ferdiom opened this issue May 6, 2024 · 5 comments · May be fixed by #26568
Open

wrong result integrating sin(a*(x+pi))**2 #26566

ferdiom opened this issue May 6, 2024 · 5 comments · May be fixed by #26568
Labels
Add test to close Issue has been fixed, add a test and it can be closed. Easy to Fix This is a good issue for new contributors. Feel free to work on this if no one else has already. integrals.definite integrals

Comments

@ferdiom
Copy link

ferdiom commented May 6, 2024

Hi,

I am using sympy version 1.12 and like some other people here, I encountered a wrong sign in a definite integral
from sympy import *
x = symbols('x', real=True)
a = symbols('x', real=True, positive=True)
integrate(sin(a*(x+pi))**2, (x, -pi, -pi/2))

it returns the result
-pi/4 + sin(pi*x)/(4*x)
which has the wrong sign.

@oscarbenjamin
Copy link
Contributor

The sign issue is already fixed on master. I bisected the fix to 352587a from gh-26173.

There are other problems though. The definite integral should not have x. The reason is because of a bug to do with replacing symbols with assumptions:

In [16]: i1 = integrate(sin(a*(x+pi))**2, (x, -pi, -pi/2))

In [17]: i2 = + pi/4 - sin(pi*x)/(4*x)

In [18]: i1
Out[18]: 
π   sin(πx)
─ - ────────
4     4x   

In [19]: i2
Out[19]: 
π   sin(πx)
─ - ────────
4     4x   

In [20]: i1 == i2
Out[20]: False

In [21]: srepr(i1)
Out[21]: "Add(Mul(Rational(1, 4), pi), Mul(Integer(-1), Rational(1, 4), Pow(Symbol('x', real=True, positive=True), Integer(-1)), sin(Mul(pi, Symbol('x', real=True, positive=True)))))"

In [22]: srepr(i2)
Out[22]: "Add(Mul(Rational(1, 4), pi), Mul(Integer(-1), Rational(1, 4), Pow(Symbol('x', real=True), Integer(-1)), sin(Mul(pi, Symbol('x', real=True)))))"

The result returned from integrate has Symbol('x', real=True, positive=True) rather than Symbol('x', real=True). Somewhere the symbol got replaced with one that has the positive assumption but then was not replaced back. Presumably this is what results in it still having x because substituting limits for the wrong symbol (with different assumptions) failed.

@ferdiom
Copy link
Author

ferdiom commented May 6, 2024

the x in the result is my error ... x and a have the same symbol in the definition. It of course should be

a = symbols('a', real=True, positive=True)
and then I get the result
-pi/4 + sin(pi*a)/(4*a)

@oscarbenjamin
Copy link
Contributor

Oh, of course. In that case this is fixed on master by the commit I referred above:

In [1]: from sympy import *
   ...: x = symbols('x', real=True)
   ...: a = symbols('a', real=True, positive=True)
   ...: integrate(sin(a*(x+pi))**2, (x, -pi, -pi/2))
Out[1]: 
π   sin(πa)
─ - ────────
4     4a   

This issue can be closed if a test is added for this case.

@oscarbenjamin oscarbenjamin added Easy to Fix This is a good issue for new contributors. Feel free to work on this if no one else has already. Add test to close Issue has been fixed, add a test and it can be closed. labels May 6, 2024
@Abhishekjsr283204
Copy link
Contributor

Abhishekjsr283204 commented May 6, 2024

@oscarbenjamin sir this test is okay in test_integrals.py
def test_definite_integral_sign_issue_fixed or test_issue_26566():
x = symbols('x', real=True)
a = symbols('a', real=True, positive=True)
assert sin(a*(x+pi))**2 == simplify(pi/4 - sin(pi * a)/(4 * a)) can i make a PR for this or some problem in that

@oscarbenjamin
Copy link
Contributor

The test looks fine to me

skirpichev added a commit to skirpichev/diofant that referenced this issue May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Add test to close Issue has been fixed, add a test and it can be closed. Easy to Fix This is a good issue for new contributors. Feel free to work on this if no one else has already. integrals.definite integrals
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants