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

Adding Tests for Geometry module #18954

Merged
merged 5 commits into from Mar 28, 2020
Merged

Adding Tests for Geometry module #18954

merged 5 commits into from Mar 28, 2020

Conversation

OmarWagih1
Copy link
Contributor

@OmarWagih1 OmarWagih1 commented Mar 25, 2020

Brief description of what is fixed or changed

This adds tests to the Geometry module to increase the test coverage.

Release Notes

NO ENTRY

@sympy-bot
Copy link

sympy-bot commented Mar 25, 2020

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

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

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

This adds tests to the Geometry module to increase the test coverage.

#### 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 -->

sympy/geometry/tests/test_ellipse.py Outdated Show resolved Hide resolved
sympy/geometry/tests/test_line.py Outdated Show resolved Hide resolved
sympy/geometry/tests/test_line.py Outdated Show resolved Hide resolved
sympy/geometry/tests/test_ellipse.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 25, 2020

Codecov Report

Merging #18954 into master will decrease coverage by 8.037%.
The diff coverage is 100%.

@@              Coverage Diff              @@
##            master    #18954       +/-   ##
=============================================
- Coverage   75.741%   67.704%   -8.038%     
=============================================
  Files          647       647               
  Lines       168584    168623       +39     
  Branches     39723     39734       +11     
=============================================
- Hits        127688    114165    -13523     
- Misses       35344     48896    +13552     
- Partials      5552      5562       +10

@@ -477,6 +477,10 @@ def test_section_modulus_and_polar_second_moment_of_area():
assert e.section_modulus() == (18*pi, 6*pi)
assert e.polar_second_moment_of_area() == 120*pi

e = Ellipse(Point(0, 0), 2, 2)
assert e.section_modulus() == (2*pi, 2*pi)
assert e.section_modulus(Point(2, 2)) == (2*pi, 2*pi)
Copy link
Member

Choose a reason for hiding this comment

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

You could test that this method accepts a 2-element tuple with e.section_modulus((2,2))...after making the change to Ellipse.section_modulus:

        else:
            # taking x and y as distances of the given point from the center
            point = Point(point)
            y = point.y - y_c

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a nice suggestion, done!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smichr I tried this suggestion (commit above) but it altered something in the output, it passes the tests on local but not on Travis CI, could you take a look at it and tell me if you see anything wrong?

@smichr smichr added the PR: author's turn The PR has been reviewed and the author needs to submit more changes. label Mar 27, 2020
Copy link
Member

@namannimmo10 namannimmo10 left a comment

Choose a reason for hiding this comment

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

There's a typo in this line, can you correct that also?

sympy/geometry/ellipse.py Outdated Show resolved Hide resolved
sympy/geometry/tests/test_ellipse.py Show resolved Hide resolved
OmarWagih1 and others added 2 commits March 28, 2020 13:20
Co-Authored-By: Naman Gera <namangera15@gmail.com>
@smichr smichr merged commit 3f305ce into sympy:master Mar 28, 2020
@smichr
Copy link
Member

smichr commented Mar 28, 2020

Thanks, this is in.

@namannimmo10
Copy link
Member

There's still a typo here, will deal with it in the next PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
geometry PR: author's turn The PR has been reviewed and the author needs to submit more changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants