Skip to content

Add Matrix6 class#455

Merged
chapulina merged 11 commits intoign-math6from
chapulina/6/mat6
Jul 8, 2022
Merged

Add Matrix6 class#455
chapulina merged 11 commits intoign-math6from
chapulina/6/mat6

Conversation

@chapulina
Copy link
Copy Markdown
Contributor

@chapulina chapulina commented Jul 6, 2022

🎉 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:

Test it

Run the added tests.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

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.

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

chapulina added 5 commits July 5, 2022 19:38
@chapulina
Backport newest appendToStream functions
5c43624 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Port main functionality from Matrix4 to Matrix6, with updates from gz…
e7668ec 
…-math7

Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Submatrix
2be91fb 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
codecheck
f29aed1 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Python
b8987a6 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina added the MBARI buoy Sponsored by MBARI buoy sim project: https://github.com/osrf/buoy_sim label Jul 6, 2022
@chapulina chapulina requested a review from quarkytale July 6, 2022 05:02
@chapulina
doxy
2e3946c 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Base automatically changed from chapulina/6/appendToStream to ign-math6 July 6, 2022 15:58
@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 6, 2022 •
edited
Loading

Codecov Report

Merging #455 (8e99bdf) into ign-math6 (2b373a5) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@              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
Impacted Files Coverage Δ
eigen3/include/ignition/math/eigen3/Conversions.hh 100.00% <100.00%> (ø)
include/ignition/math/Matrix6.hh 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b373a5...8e99bdf. Read the comment docs.

@chapulina chapulina added 🏰 citadel Ignition Citadel 🏯 fortress Ignition Fortress scripting Scripting interfaces to Ignition labels Jul 6, 2022
@chapulina chapulina self-assigned this Jul 6, 2022
scpeters
scpeters reviewed Jul 6, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@scpeters scpeters left a 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?

Comment thread include/ignition/math/Matrix6.hh Outdated
Comment thread include/ignition/math/Matrix6.hh Outdated
Comment thread include/ignition/math/Matrix6.hh Outdated
Comment thread include/ignition/math/Matrix6.hh Outdated
Comment thread include/ignition/math/Matrix6.hh Outdated
Comment thread include/ignition/math/Matrix6.hh Outdated
Comment thread include/ignition/math/Matrix6.hh Outdated
Comment thread 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));
Copy link
Copy Markdown
Member

@scpeters scpeters Jul 6, 2022

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

Copy link
Copy Markdown
Contributor Author

@chapulina chapulina Jul 7, 2022

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.

Comment thread include/ignition/math/Matrix6.hh Outdated
chapulina added 2 commits July 7, 2022 10:05
@chapulina
Reduce duplication in multiplication, add SetSubmatrix
efb0f0d 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
doxy
f12eedc 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Copy link
Copy Markdown
Contributor Author

chapulina commented Jul 7, 2022

what do you think about adding a SetSubMatrix(Matrix6Corner, Matrix3) method?

Good idea, this will probably be useful. Added in efb0f0d

@chapulina
eigen conversion
030e2c1 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
scpeters
scpeters approved these changes Jul 7, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@scpeters scpeters left a 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)

@scpeters
Fix line length
8e99bdf 
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@chapulina chapulina merged commit 3d47035 into ign-math6 Jul 8, 2022
@chapulina chapulina deleted the chapulina/6/mat6 branch July 8, 2022 16:20
@chapulina chapulina mentioned this pull request Jul 11, 2022
Merged
11 tasks
@chapulina chapulina mentioned this pull request Jul 20, 2022
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@scpeters scpeters scpeters approved these changes

@quarkytale quarkytale Awaiting requested review from quarkytale

@adityapande-1995 adityapande-1995 Awaiting requested review from adityapande-1995 adityapande-1995 is a code owner

Assignees

@chapulina chapulina

Labels

🏰 citadel Ignition Citadel 🏯 fortress Ignition Fortress MBARI buoy Sponsored by MBARI buoy sim project: https://github.com/osrf/buoy_sim scripting Scripting interfaces to Ignition

Projects

Core development
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@chapulina @scpeters @osrf-triage