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

Tests for center_of_mass and _w_diff_dcm #15013

Merged
merged 5 commits into from Aug 22, 2018

Conversation

Projects
None yet
3 participants
@NikhilPappu
Copy link
Contributor

NikhilPappu commented Aug 2, 2018

  • physics.mechanics
    • Added a test for the center_of_mass() function in test_functions.py.
  • physics.vector
    • Added a test for _w_diff_dcm in test_frame.py.
@sympy-bot

This comment has been minimized.

Copy link

sympy-bot commented Aug 2, 2018

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

  • physics.mechanics

    • Added a test for the center_of_mass() function in test_functions.py. (#15013 by @NikhilPappu)
  • physics.vector

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.2.1.

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.

<!-- BEGIN RELEASE NOTES -->
- physics.mechanics
   - Added a test for the center_of_mass() function in test_functions.py.
- physics.vector
   - Added a test for _w_diff_dcm in test_frame.py.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

@moorepants
Copy link
Member

moorepants left a comment

This looks good. It is just missing some explanations of why these tests were added and some more info explaining the tests. I'm also not sure why the example was changed.


def test_center_of_mass():
from sympy import symbols, S
from sympy.physics.mechanics.functions import center_of_mass

This comment has been minimized.

@moorepants

moorepants Aug 18, 2018

Member

Imports should be at top of file unless there are circular dependencies.

@@ -198,3 +198,26 @@ def test_gravity():
for i in range(len(l)):
for j in range(len(l[i])):
assert forceList[i][j] == l[i][j]


def test_center_of_mass():

This comment has been minimized.

@moorepants

moorepants Aug 18, 2018

Member

Can you add a comment explaining what exactly this is testing and reference an issue where the bug was found?

This comment has been minimized.

@moorepants

moorepants Aug 18, 2018

Member

Is this an example that comes from one of the autolev texts?

This comment has been minimized.

@NikhilPappu

NikhilPappu Aug 18, 2018

Author Contributor

This is the test for the center of mass function I had previously added but did not include a test for. There is no issue associated with it.

This comment has been minimized.

@NikhilPappu

NikhilPappu Aug 18, 2018

Author Contributor

The example isn't from an Autolev text.

This comment has been minimized.

@moorepants

moorepants Aug 21, 2018

Member

Ok, please add a comment to the code explaining what it is testing and reference where you found the bug.

@@ -158,6 +158,23 @@ def test_dcm():
cos(q1)*cos(q2)]])


def test_w_diff_dcm():
from sympy.physics.vector import dot

This comment has been minimized.

@moorepants

moorepants Aug 18, 2018

Member

imports at top

>>> point_so=Point('so')
>>> point_so.set_pos(p1.point, center_of_mass(p1.point, p1, p2, p3, p4, p5, p6, p7, p8))
>>> point_so.pos_from(p1.point)
18/(mu + 28)*a.x + (mu + 14)/(mu + 28)*a.y + (mu + 18)/(mu + 28)*a.z

This comment has been minimized.

@moorepants

moorepants Aug 18, 2018

Member

So I'm assuming this example gave incorrect output?

This comment has been minimized.

@moorepants

This comment has been minimized.

@NikhilPappu

NikhilPappu Aug 21, 2018

Author Contributor

This example didn't give any incorrect output. I changed it because it was from an Autolev text.

>>> point_o.set_pos(p1.point, center_of_mass(p1.point, p1, p2, p3, p4, b))
>>> expr = 5/(m + mb + 6)*a.x + (m + mb + 3)/(m + mb + 6)*a.y + mb/(m + mb + 6)*a.z
>>> point_o.pos_from(p1.point)
5/(m + mb + 6)*a.x + (m + mb + 3)/(m + mb + 6)*a.y + mb/(m + mb + 6)*a.z

This comment has been minimized.

@moorepants

moorepants Aug 18, 2018

Member

The previous example had like eight particles. This seems to have fewer. Why is it changed?

This comment has been minimized.

@NikhilPappu

NikhilPappu Aug 18, 2018

Author Contributor

The previous example was from an Autolev text so I changed it. Fewer particles looks better I guess. I also added a rigidbody here which wasn't a part of the previous one.

@@ -158,6 +158,23 @@ def test_dcm():
cos(q1)*cos(q2)]])


def test_w_diff_dcm():

This comment has been minimized.

@moorepants

moorepants Aug 18, 2018

Member

Please add a comment explaining what bug this is testing and a reference issue where it was discovered.

This comment has been minimized.

@NikhilPappu

NikhilPappu Aug 18, 2018

Author Contributor

w_diff_dcm gives results which don't match with the Autolev ones so I am assuming they are incorrect. Also, there were no tests for this so I think it wasn't checked properly if it works.
I am not sure why my change is correct but it seems to work. Should I open an issue for it?

This comment has been minimized.

@moorepants

moorepants Aug 21, 2018

Member

I just want some explanation in the code so that future authors can understand what happened.

b3a=(b.z).express(a)
b.set_ang_vel(a, b.x*(dot((b3a).dt(a), b.y)) + b.y*(dot((b1a).dt(a), b.z)) +
b.z*(dot((b2a).dt(a), b.x)))
expr = (c12*c13d + c22*c23d + c32*c33d)*b.x + (c13*c11d + c23*c21d + c33*c31d)*b.y +\

This comment has been minimized.

@moorepants

moorepants Aug 18, 2018

Member

no backslash needed

This comment has been minimized.

@moorepants

moorepants Aug 18, 2018

Member

use parantheses instead of backslashs

@NikhilPappu

This comment has been minimized.

Copy link
Contributor Author

NikhilPappu commented Aug 18, 2018

@moorepants There wasn't any problem with the previous example. I changed it because it was from one of the Autolev texts. As for w_diff_dcm, it seems to give the wrong result when compared with the Autolev results. I am not sure why my change is correct but it seems to work. Should I open an issue for it? There were no test cases written for this previously.

NikhilPappu added some commits Aug 18, 2018

@NikhilPappu

This comment has been minimized.

Copy link
Contributor Author

NikhilPappu commented Aug 22, 2018

@moorepants I added the comments.

@moorepants

This comment has been minimized.

Copy link
Member

moorepants commented Aug 22, 2018

Looks good!

@moorepants moorepants merged commit c6cf691 into sympy:master Aug 22, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
sympy-bot/release-notes The release notes look OK
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.