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

[GSoC] Corrected some limit XFAIL tests #17381

Open
wants to merge 2 commits into
base: master
from

Conversation

@ArighnaIITG
Copy link
Member

commented Aug 11, 2019

References to other Issues or PRs

Fixes #17380

Brief description of what is fixed or changed

Previously,

>>> from sympy import *
>>> x = symbols('x')
>>> limit(x*(((x + 1)**2 + 1)/(x**2 + 1) - 1), x, oo)
0

Now,

>>> from sympy import *
>>> x = symbols('x')
>>> limit(x*(((x + 1)**2 + 1)/(x**2 + 1) - 1), x, oo)
2

More tests can be found in the related issues and the code commits.

Other comments

Release Notes

  • series
    • Corrected XFAIL tests in sympy.series.limits.
@sympy-bot

This comment has been minimized.

Copy link

commented Aug 11, 2019

Hi, I am the SymPy bot (v147). 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.5.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs

Fixes #17380 

#### Brief description of what is fixed or changed

Previously,
```
>>> from sympy import *
>>> x = symbols('x')
>>> limit(x*(((x + 1)**2 + 1)/(x**2 + 1) - 1), x, oo)
0
```

Now,

```
>>> from sympy import *
>>> x = symbols('x')
>>> limit(x*(((x + 1)**2 + 1)/(x**2 + 1) - 1), x, oo)
2
```

More tests can be found in the related issues and the code commits.

#### Other comments


#### Release Notes

<!-- Write the release notes for this release below. 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 -->
* series
   * Corrected XFAIL tests in `sympy.series.limits`.
<!-- END RELEASE NOTES -->

@ArighnaIITG ArighnaIITG changed the title Introduced new_expr Corrected some limit XFAIL tests Aug 11, 2019
@ArighnaIITG ArighnaIITG changed the title Corrected some limit XFAIL tests [GSoC] Corrected some limit XFAIL tests Aug 11, 2019
@ArighnaIITG

This comment has been minimized.

Copy link
Member Author

commented Aug 11, 2019

Ping @leosartaj

@codecov

This comment has been minimized.

Copy link

commented Aug 11, 2019

Codecov Report

Merging #17381 into master will increase coverage by 0.004%.
The diff coverage is 94.444%.

@@              Coverage Diff              @@
##            master    #17381       +/-   ##
=============================================
+ Coverage   74.696%   74.701%   +0.004%     
=============================================
  Files          633       633               
  Lines       164405    164421       +16     
  Branches     38593     38602        +9     
=============================================
+ Hits        122805    122825       +20     
+ Misses       36215     36207        -8     
- Partials      5385      5389        +4
@leosartaj

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

Codecov Report

Merging #17381 into master will decrease coverage by 7.822%.
The diff coverage is 94.444%.

@@              Coverage Diff              @@
##            master    #17381       +/-   ##
=============================================
- Coverage   74.669%   66.846%   -7.823%     
=============================================
  Files          631       631               
  Lines       163172    163188       +16     
  Branches     38282     38291        +9     
=============================================
- Hits        121840    109086    -12754     
- Misses       35996     48753    +12757     
- Partials      5336      5349       +13

You might want to check why coverage reduced.


if compute:
# simple leading term analysis gave us cancelled terms whose sum is 0
# so compute the leading term (via series)

This comment has been minimized.

Copy link
@leosartaj

leosartaj Aug 12, 2019

Member

I am confused on this step. if compute is True old.compute_leading_term(x) is returned, where is expr supposed to be used then?

This comment has been minimized.

Copy link
@ArighnaIITG

ArighnaIITG Aug 14, 2019

Author Member

I will explain it to you.

old.compute_leading_term(x) is executed when the leading terms of expr.args cancel each other out perfectly. So expr = expr.func(*[t.as_leading_term(x) for t in expr.args]).removeO() turns expr into 0.
But the current implementation does not check whether the leading terms cancelled each other partially.So instead of expr, new_expr is introduced new_expr = expr.func(*[t.as_leading_term(x) for t in expr.args]).removeO().

Then number of arguments of expr and new_expr are checked. If they are not equal, i.e., compute is True, (old.compute_leading_term(x) is used. The following code will use new_expr now, not expr.

This comment has been minimized.

Copy link
@jksuom

jksuom Aug 14, 2019

Member

Then number of arguments of expr and new_expr are checked.

This is probably not helpful in general. The leading terms in new_expr have different orders. Only those of the lowest (or dominating) order matter. For example, assume that there are two arguments a0, a1 of order 0 and two arguments b0, b1 of order 1. If the constant leading terms of a0 and a1 do not cancel, then the sum has order 0 and it does not matter at all if the leading terms of b0 and b1 cancel or not. On the other hand, if the constant terms cancel each other, then the first order terms become significant. But those of b0 and b1 do not suffice alone, the first order terms of a0 and a1 need to be computed as well.

So it seems to me that the following would be the proper procedure. Compute the list of all leading terms of the arguments. Filter the list omitting all but the dominating terms. Compute the sum of those terms and return it if it is not zero. Otherwise pass to old.compute_leading_term().

This comment has been minimized.

Copy link
@leosartaj

leosartaj Aug 19, 2019

Member

Hi @ArighnaIITG Any updates on this?

This comment has been minimized.

Copy link
@ArighnaIITG

ArighnaIITG Aug 23, 2019

Author Member

@jksuom I am not being able to understand your point. Could you please explain?

This comment has been minimized.

Copy link
@jksuom

jksuom Sep 30, 2019

Member

Maybe the filtering part is not clear. What I mean is this:
Make a list of the leading terms of the arguments. Then make another list of their orders as integers. Find the minimum of the orders. From the list of leading terms, remove all terms whose order is greater than the minimum. Compute the sum of the remaining leading terms all of which have the same order. If the sum is non-zero, then it is the leading term and can be returned. Otherwise pass to old.compute_leading_term().

@oscargus

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

You might want to check why coverage reduced.

This could most likely be blamed on CodeCov. It is a bit flaky.

What could be interesting from a test perspective is to trigger the S.NaN paths.

@ArighnaIITG Have you looked into any of the other limit issues and seen if this may fix any of these? It may also be an idea to add "all" reported issues as XFAIL-tests for later reference. (Given that you have time etc.)

@oscargus

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

Btw, I realize that I added some failing limit tests in #17390.

@leosartaj

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

Btw, I realize that I added some failing limit tests in #17390.

Hi @ArighnaIITG can you take a quick look, if anything else is also fixed.

@oscargus oscargus added this to the SymPy 1.5 milestone Aug 13, 2019
@ArighnaIITG

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

@oscargus @leosartaj I have all other limit XFAIL tests, and currently only the crop of tests I mentioned, seems to be the ones passing. Others are getting failed still.

@leosartaj

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

You have merge conflicts. Please fix them.

@ArighnaIITG

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2019

You have merge conflicts. Please fix them.

Merge conflicts have been fixed. @leosartaj

@oscarbenjamin

This comment has been minimized.

Copy link
Contributor

commented Sep 29, 2019

This needs a review.

CC @leosartaj @jksuom

# back a term, so compute the leading term (via series)
compute = False
leading_terms = [t.as_leading_term(x) for t in expr.args]
new_expr = expr.func(*leading_terms).removeO()

This comment has been minimized.

Copy link
@jksuom

jksuom Sep 30, 2019

Member

It is important that only leading terms of lowest order are included here. For example, if the expression is 1/(1 - x) - 1 + x**2, then the leading terms are [1, -1, x**2] and their sum x**2 is non-zero. However, the series of the expression is x + 2*x**2 + ..., which shows that the correct leading term is x. That can be found if new_expr is computed as the sum of the lowest order leading terms [1, -1].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.