-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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] Concrete: Implemented Raabe's Test #18656
Conversation
✅ Hi, I am the SymPy bot (v160). 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.7. 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.
Update The release notes on the wiki have been updated. |
Please suggest some regression tests which could be added. |
@@ -512,6 +512,21 @@ def is_convergent(self): | |||
except NotImplementedError: | |||
pass | |||
|
|||
### ---------- Raabe's test -------------- ### | |||
if lim_ratio == 1: # ratio test inconclusive |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a discussion in the other pull request about whether or not this is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo it is necessary...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you talking about raabe's test or the condition??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition. Your answer above says "imo it is necessary". I am sure that I agree but I would prefer it if you write an explicit reason (an opinion is not a reason).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We implement Raabe's test only after ratio test fails, and the condition when ratio test fails is when it is 1, so definitely it is necessary, otherwise how will we come to know when to implement raabe's test.
try: | ||
lim_val = limit_seq(test_val, sym) | ||
if lim_val is not None and lim_val.is_number: | ||
if lim_val > 1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can check (lim_val - 1).is_positive
here, to allow it to potentially work for symbols with the proper assumptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK will change the line if lim_value > 1:
What about this @asmeurer |
I don't know. You might need to google around for some examples. I did find https://arxiv.org/abs/1801.07584, which has some. You can also look for series that are known to fail the ratio test. |
Series like these are giving maximum recursion depth exceeded: But they should pass with raabe's test. The point is they are not able to reach up to the point where raabe's test is implemented as the depth is exceeded in the divergence tests itself. |
Moreover |
@jksuom @asmeurer ?? And these are the sort of examples where raabe's test becomes useful. |
It looks like divergence test should be fixed first so it won't return false when Then Raabe's test should be able to handle sums of |
Now imo, most of the series should no longer cause any RecursionError but I feel there are chances of wrong answers as limit_seq sometimes returns wrong limit values (Ex:#16735) |
Codecov Report
@@ Coverage Diff @@
## master #18656 +/- ##
==============================================
- Coverage 75.814% 59.567% -16.247%
==============================================
Files 669 669
Lines 173463 173475 +12
Branches 40907 40911 +4
==============================================
- Hits 131510 103335 -28175
- Misses 36230 64290 +28060
- Partials 5723 5850 +127 |
6372519
to
b45bc4f
Compare
Thanks, I think this is ready to go. |
Reference: #12352
Fixes: #19379
Fixes: #10973
Brief description of what is fixed or changed
This pull request adds Raabe's Test. It is an enhancement of ratio test that is intended for the case where it is inconclusive.
Other comments
Regression Test added
Release Notes