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.

Sign up for GitHub

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

Jump to bottom

Increase test coverage in series.py #20945

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Increase test coverage in series.py #20945

wants to merge 10 commits into from

Conversation

ayushbisht2001
Copy link
Contributor

Added more tests for series.

References to other Issues or PRs

Refer: #16318

Brief description of what is fixed or changed

Other comments

Release Notes

NO ENTRY

@sympy-bot
Copy link

✅

Hi, I am the SymPy bot (v161). 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.
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. -->
Added more tests for series.
#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->
Refer: #16318

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


#### 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 -->
NO ENTRY
<!-- END RELEASE NOTES -->

@ayushbisht2001
Copy link
Contributor Author

ayushbisht2001 commented Feb 12, 2021 •
edited

@oscargus , I tried to add more test cases for series. Any suggestion will be highly appreciated.

0sidharth
0sidharth reviewed Feb 13, 2021
View reviewed changes
sympy/series/tests/test_series.py Outdated
@@ -17,6 +17,30 @@ def test_cos():
assert e1 == e2


def test_tan():
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to merge all these tests and test_sin and test_cos into one test_trigonometric? Same for the multiple test_exp functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @0sidharth , now its looks good..

@ayushbisht2001
Copy link
Contributor Author

@jksuom , can you please check this PR, Whenever you get time...

0sidharth
0sidharth reviewed Feb 16, 2021
View reviewed changes
sympy/solvers/tests/test_solvers.py Outdated


def test_issue_4886():
Ra , Rb = var('Ra , Rb',positive = True)
Copy link
Member

@0sidharth 0sidharth Feb 16, 2021 •
edited

Choose a reason for hiding this comment

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

Do you need to use pow? Why not m = a**2 + b**2

var should not be used in library code. Use symbols instead.

Follow PEP8 conventions throughout, a few pointers -
There should be a space after ,, and no space before, there should be spaces around + and -, assignment operator should not have extra leading spaces to make it aligned, only one space before and one after is fine, assigning values to function parameters should not have any spaces around assignment operator (positive=True)

@0sidharth
Copy link
Member

You also need to resolve the conflicts so that the tests can run

@ayushbisht2001
Copy link
Contributor Author

You also need to resolve the conflicts so that the tests can run

@0sidharth , can u pls help me .. how can I resolve these conflicts??

@Smit-create
Copy link
Member

how can I resolve these conflicts??

Refer this: #18152 (comment)

@ayushbisht2001
Revert "add tests"
c6ad5f8 
This reverts commit 782efd5.
@ayushbisht2001
Copy link
Contributor Author

@oscarbenjamin , please review this PR ...

oscarbenjamin
oscarbenjamin reviewed Mar 14, 2021
View reviewed changes
sympy/series/tests/test_series.py
e1 = cos(x).series(x, 0)
e2 = series(cos(x), x, 0)
assert e1 == e2

e1 = tan(x).series(x, 0)
e2 = series(tan(x), x, 0)
assert e1 == e2
Copy link
Contributor

Choose a reason for hiding this comment

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

These tests don't seem very interesting. How exactly do they increase coverage?

I would have thought that a better test would actually show what the series should be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oscarbenjamin oscarbenjamin oscarbenjamin left review comments

@0sidharth 0sidharth 0sidharth left review comments

Assignees
No one assigned
Labels
Merge conflict series
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ayushbisht2001 @sympy-bot @0sidharth @Smit-create @oscarbenjamin