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

Add test for limit_seq in series #19876

Merged
merged 1 commit into from
Aug 1, 2020
Merged

Add test for limit_seq in series #19876

merged 1 commit into from
Aug 1, 2020

Conversation

tashakim
Copy link
Contributor

@tashakim tashakim commented Aug 1, 2020

####References to other Issues or PRs
Fixes #19868

Brief description of what is fixed or changed

Added simple test case for limit_seq, to ensure correct output of 0 instead of oo.
Current master gives 0.

Other comments

NA

Release Notes

NO ENTRY

@sympy-bot
Copy link

sympy-bot commented Aug 1, 2020

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.

  • No release notes entry will be added for this pull request.

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.

####References to other Issues or PRs
Fixes #19868 

#### Brief description of what is fixed or changed
Added simple test case for limit_seq, to ensure correct output of 0 instead of oo.
Current master gives 0.

#### Other comments
NA

#### Release Notes
<!-- BEGIN RELEASE NOTES -->
NO ENTRY
<!-- END RELEASE NOTES -->

Comment on lines 86 to 87
# issue 19868
assert limit_seq(1/gamma(n+S.One/2), n) == 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be like this:

def test_issue_19868():
    assert limit_seq(1/gamma(n + S.One/2), n) == 0

Copy link
Contributor

@sachin-4099 sachin-4099 Aug 1, 2020

Choose a reason for hiding this comment

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

Also maintain two line spacing between the testcases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, thanks

@tashakim
Copy link
Contributor Author

tashakim commented Aug 1, 2020

Fixed format of testcase as guided. Please review

@@ -133,6 +133,10 @@ def test_issue_16735():
assert limit_seq(5**n/factorial(n), n) == 0


def test_issue_19868():
assert limit_seq(1/gamma(n + S.One/2), n) == 0

Copy link
Contributor

Choose a reason for hiding this comment

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

This line contains trailing whitespaces. Line number 138.

@tashakim
Copy link
Contributor Author

tashakim commented Aug 1, 2020

Fixed, thanks

@sachin-4099
Copy link
Contributor

Add gamma to the imports to line number 2 of test_limitseq.py. I will have to request you to squash all the commits and push a single commit.

@tashakim
Copy link
Contributor Author

tashakim commented Aug 1, 2020

Added gamma to imports. Could you please give me guidance on how to squash the commits?

@tashakim
Copy link
Contributor Author

tashakim commented Aug 1, 2020

Done. Please review

@codecov
Copy link

codecov bot commented Aug 1, 2020

Codecov Report

Merging #19876 into master will increase coverage by 0.038%.
The diff coverage is 86.666%.

@@              Coverage Diff              @@
##            master    #19876       +/-   ##
=============================================
+ Coverage   75.703%   75.741%   +0.038%     
=============================================
  Files          662       667        +5     
  Lines       172162    172738      +576     
  Branches     40604     40720      +116     
=============================================
+ Hits        130332    130835      +503     
- Misses       36138     36178       +40     
- Partials      5692      5725       +33     

@sachin-4099
Copy link
Contributor

Looks good to me. This can be merged @jksuom.

@sachin-4099 sachin-4099 requested a review from jksuom August 1, 2020 17:05
@jksuom
Copy link
Member

jksuom commented Aug 1, 2020

Thanks.

@jksuom jksuom merged commit db6529b into sympy:master Aug 1, 2020
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.

Bug in limit_seq
4 participants