-
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
fix dcm diff error #16828
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
fix dcm diff error #16828
Conversation
|
✅ Hi, I am the SymPy bot (v147). 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:
This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.5. 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.
Update The release notes on the wiki have been updated. |
Codecov Report
@@ Coverage Diff @@
## master #16828 +/- ##
=============================================
+ Coverage 73.849% 73.864% +0.014%
=============================================
Files 619 619
Lines 159669 159712 +43
Branches 37476 37486 +10
=============================================
+ Hits 117915 117970 +55
+ Misses 36294 36281 -13
- Partials 5460 5461 +1 |
|
This looks reasonable although I haven't checked the calculation in detail. Was it just a bug in the formula? Can you add a test for this? The example from #16824 could be added to sympy/physics/vector/tests/test_frame.py |
|
@oscarbenjamin, the test function has been added. Please check it. Two test functions |
| DCM = C.dcm(N).T | ||
| D = N.orientnew('D', 'DCM', DCM) | ||
|
|
||
| assert D.dcm(N) == C.dcm(N) |
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.
Can you also include explicitly what this is testing for rather than just comparing the output of two different functions?
Otherwise this test will pass even if both just output e.g. zero.
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.
I have added some documentation for both test functions and rewritten the original test_w_diff_dcm1() since it looks not good.
Co-Authored-By: Christopher Smith <smichr@gmail.com>
|
Thanks! |
References to other Issues or PRs
Fix ReferenceFrame DCM diff error #16824
Brief description of what is fixed or changed
Fix angular velocity from time differentiating the DCM.
Result:
Other comments
Release Notes
_w_diff_dcmmethod