-
Notifications
You must be signed in to change notification settings - Fork 67
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
Add Matrix6 class #455
Add Matrix6 class #455
Conversation
Signed-off-by: Louise Poubel <[email protected]>
…-math7 Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Codecov Report
@@ Coverage Diff @@
## ign-math6 #455 +/- ##
=============================================
+ Coverage 99.65% 99.66% +0.01%
=============================================
Files 67 68 +1
Lines 6400 6614 +214
=============================================
+ Hits 6378 6592 +214
Misses 22 22
Continue to review full report at Codecov.
|
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've made a few minor comments, aside from thinking that the multiplication operator would be better off with for loops, since it's so long.
Also, what do you think about adding a SetSubMatrix(Matrix6Corner, Matrix3)
method?
include/ignition/math/Matrix6.hh
Outdated
this->data[5][2] * _m2(2, 5) + | ||
this->data[5][3] * _m2(3, 5) + | ||
this->data[5][4] * _m2(4, 5) + | ||
this->data[5][5] * _m2(5, 5)); |
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.
there may be a performance advantage to manually unrolling the loops, but this is a bit too much; I'd rather just have two for loops here
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 was trying to keep the diff to Matrix4
as little as possible, so I followed the pattern there. But I agree with you that this is very error-prone. See efb0f0d for a more compact implementation which doesn't require allocating the matrix with zeroes and then changing each element one by one.
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Good idea, this will probably be useful. Added in efb0f0d |
Signed-off-by: Louise Poubel <[email protected]>
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 with clean CI (there are a few lines over 80 characters)
Signed-off-by: Steve Peters <[email protected]>
🎉 New feature
Summary
Add a new
Matrix6
class to be used to store added mass (see gazebosim/gz-sim#1462).The class is an extension of the existing
Matrix4
class, with these differences:Matrix4
onmain
:Determinant
andInverse
Submatrix
function, as requested in Add MatrixX class #442 (comment)main
, by usingGZ_
header guards, removing redundantignition::math::
namespaces, etc.Test it
Run the added tests.
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-by
messages.🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸