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

Improper integral does not evaluate #10445 #10515

Merged
merged 4 commits into from
Feb 3, 2016

Conversation

shubhamtibra
Copy link
Contributor

Modified the code to compute the limit of antiderivative when upper-limit is Infinity. #10445

Modified the code to compute the limit of antiderivative when upper-limit is Infinity.
@@ -520,8 +521,14 @@ def try_meijerg(function, xab):
function = antideriv._eval_interval(x, a, b)
function = Poly(function, *gens)
elif isinstance(antideriv, Add):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this alternative should be entirely removed. The whole antideriv could be passed to _eval_interval as is done after the next else: statement.

@shubhamtibra
Copy link
Contributor Author

Some test are failing. I can't figure it out, @jksuom can you take a look at it?

else:
try:
function = antideriv._eval_interval(x, a, b)
if b is S.Infinity:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test and the following lines are not necessary as the limit is taken into account in _eval_interval. They seem to change the result in a way that makes some tests fail.

@jksuom
Copy link
Member

jksuom commented Feb 3, 2016

The failures seem to be caused by the lines containing c. You should remove those and then test locally with bin/test (at least bin/test stats).

@shubhamtibra
Copy link
Contributor Author

What should i use instead of c? Is there a way doing it without creating a new symbol?

@jksuom
Copy link
Member

jksuom commented Feb 3, 2016

The issue is not the symbol name. All those lines can be removed. It is not necessary to consider limits at this point.

@shubhamtibra
Copy link
Contributor Author

I did that but now the integral gives nan, are we supposed leave the bug then?

@jksuom
Copy link
Member

jksuom commented Feb 3, 2016

Hmm.. That is odd. I get this with your latest PR.

In [1]: integrate(1/(1+exp(x)),(x,0,oo))
Out[1]: log(2)

Could there be a difference in our systems.

@shubhamtibra
Copy link
Contributor Author

Now this test is failing assert integrate(f + exp(z), (z, 2, 3)) == integral_f - exp(2) + exp(3).

@jksuom
Copy link
Member

jksuom commented Feb 3, 2016

That is true. But that is just an inconvenience, not a real bug. The assert test can be modified, though I may have to look more closely into the associated issue before doing that.

@jksuom
Copy link
Member

jksuom commented Feb 3, 2016

The lines concerned with Add were added in commit 6fe73a1 in order to solve issue #2708, but its impact on integrals like the one in this issue were not considered. It is probably best to add @XFAIL above the test until issue #2708 is cleared up.

@shubhamtibra
Copy link
Contributor Author

I am still curious why those lines having c were causing that error?

@jksuom
Copy link
Member

jksuom commented Feb 3, 2016

Some integrals cannot be evaluated and raise a NotImplementedError exception. In that case the unevaluated integral containing the local variable c leaks out via the except statement skipping the limit line and corrupts the result.

@jksuom
Copy link
Member

jksuom commented Feb 3, 2016

Thanks! I'll merge this in a few hours if there are no objections.

@shubhamtibra
Copy link
Contributor Author

Thanks for all the help.

jksuom added a commit that referenced this pull request Feb 3, 2016
@jksuom jksuom merged commit 628fcbe into sympy:master Feb 3, 2016
@shubhamtibra shubhamtibra deleted the 10445_Improper-Integral branch February 5, 2016 20:51
jksuom added a commit to jksuom/sympy that referenced this pull request Feb 8, 2016
Issue sympy#2708 was (partially) fixed in commit 6fe73a1 by evaluating
separately the terms of the antiderivative. That gave rise to another
issue sympy#10445, which was fixed in sympy#10515 by removing the separation.

This commit aims to combine the merits of the two PRs by treating
separately those terms of the antiderivative that contain unevaluated
integral factors and evaluating the rest together in one sum.

Add tests for issue 2708
skirpichev pushed a commit to skirpichev/diofant that referenced this pull request Jan 25, 2018
Issue sympy/sympy#2708 was (partially) fixed in commit
6fe73a1 by evaluating separately the terms of the antiderivative.
That gave rise to another issue sympy/sympy#10445, which was
fixed in sympy/sympy#10515 by removing the separation.

This commit aims to combine the merits of the two PRs by treating
separately those terms of the antiderivative that contain unevaluated
integral factors and evaluating the rest together in one sum.

// edited by skirpichev

Also handle case empty factors in dup_factor_list()
skirpichev pushed a commit to skirpichev/diofant that referenced this pull request Jan 29, 2018
Issue sympy/sympy#2708 was (partially) fixed in commit
6fe73a1 by evaluating separately the terms of the antiderivative.
That gave rise to another issue sympy/sympy#10445, which was
fixed in sympy/sympy#10515 by removing the separation.

This commit aims to combine the merits of the two PRs by treating
separately those terms of the antiderivative that contain unevaluated
integral factors and evaluating the rest together in one sum.

// edited by skirpichev

Also handle case empty factors in dup_factor_list()
skirpichev pushed a commit to skirpichev/diofant that referenced this pull request Feb 1, 2018
Issue sympy/sympy#2708 was (partially) fixed in commit
6fe73a1 by evaluating separately the terms of the antiderivative.
That gave rise to another issue sympy/sympy#10445, which was
fixed in sympy/sympy#10515 by removing the separation.

This commit aims to combine the merits of the two PRs by treating
separately those terms of the antiderivative that contain unevaluated
integral factors and evaluating the rest together in one sum.

// edited by skirpichev

Also handle case of empty factors in dup_factor_list()

Signed-off-by: Sergey B Kirpichev <skirpichev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants