-
Notifications
You must be signed in to change notification settings - Fork 111
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
Contact Unit tests authenticated #274
Contact Unit tests authenticated #274
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## update-0.3.2 #274 +/- ##
================================================
- Coverage 94.72% 94.17% -0.55%
================================================
Files 43 43
Lines 2938 2938
Branches 341 341
================================================
- Hits 2783 2767 -16
- Misses 122 134 +12
- Partials 33 37 +4
☔ View full report in Codecov by Sentry. |
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.
Minor doubt.
…com/Rahul-JOON/PyElastica into 271_Contact_unit_tests_authenticity
@Rahul-JOON I will review once again, once @armantekinalp and @Ali-7800 s comments have been addressed. |
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.
Some typos
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.
LGTM
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.
LGTM
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.
LGTM
This PR aims to solve issue #271.
The following changes were made:
- corrected names of variables
- for the '_find_min_dist' test, arbitrary close lines and away lines were combined into a single test and test case for
parallel lines were added
- point to be noted: find min dist function returns -1 for contact point of system 1 if the lines intersect
- Common rod and cylinder were initialized for tests.
- test only assess the working of function with contact forces and many initial conditions were added (ex: stationary
systems and zero initial forces) for easier calculations.
- correct initialization of rod and rigidbody.
- test values were copied from specific function pytests.
If the tests are acceptable, we can make more tests that are not 'nu' independent.
Also, can we remove the elif block at https://github.com/GazzolaLab/PyElastica/blob/master/elastica/joint.py/#L574, I think this code is redundant as 'i' never equals 'n_points'.