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.

Sign up for GitHub

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

Jump to bottom

Add fluid added mass to inertial #271

Merged
merged 9 commits into from
Jul 29, 2022
Merged

Add fluid added mass to inertial #271

merged 9 commits into from
Jul 29, 2022

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented Jul 13, 2022 •
edited
Loading

🎉 New feature

Summary

Added fluid added mass support to the inertial message.

Test it

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 chapulina added the MBARI-LRAUV Sponsored by MBARI-LRAUV project: https://github.com/osrf/lrauv label Jul 13, 2022
@chapulina chapulina requested a review from caguero as a code owner July 13, 2022 21:59
@chapulina chapulina requested a review from scpeters July 13, 2022 22:00
@chapulina chapulina marked this pull request as draft July 13, 2022 22:00
@codecov
Copy link

codecov bot commented Jul 13, 2022 •
edited
Loading

Codecov Report

Merging #271 (dee433e) into main (8194ad6) will increase coverage by 0.33%.
The diff coverage is 100.00%.

❗ Current head dee433e differs from pull request most recent head 1a6a3ad. Consider uploading reports for the commit 1a6a3ad to get more accurate results

@@            Coverage Diff             @@
##             main     #271      +/-   ##
==========================================
+ Coverage   84.62%   84.96%   +0.33%     
==========================================
  Files          10       10              
  Lines         995     1024      +29     
==========================================
+ Hits          842      870      +28     
- Misses        153      154       +1
Impacted Files Coverage Δ
src/Utility.cc 83.16% <100.00%> (+0.59%) ⬆️

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 8194ad6...1a6a3ad. Read the comment docs.

@github-actions github-actions bot added the 🌱 garden Ignition Garden label Jul 13, 2022
@chapulina chapulina mentioned this pull request Jul 14, 2022
Merged
13 tasks
@chapulina chapulina marked this pull request as ready for review July 18, 2022 23:59
@chapulina chapulina requested a review from quarkytale July 18, 2022 23:59
@chapulina chapulina added the needs upstream release Blocked by a release of an upstream library label Jul 19, 2022
chapulina added 2 commits July 19, 2022 08:10
@chapulina
Conversion
5a63351 
Signed-off-by: Louise Poubel <[email protected]>
@chapulina
merged from main
b50a526 
Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina added MBARI buoy Sponsored by MBARI buoy sim project: https://github.com/osrf/buoy_sim and removed MBARI-LRAUV Sponsored by MBARI-LRAUV project: https://github.com/osrf/lrauv labels Jul 19, 2022
@chapulina chapulina added the Breaking change Breaks API, ABI or behavior. Must target unstable version. label Jul 25, 2022
@chapulina chapulina added the enhancement New feature or request label Jul 25, 2022
@chapulina chapulina self-assigned this Jul 25, 2022
@chapulina
units
1d9937d 
Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina removed the needs upstream release Blocked by a release of an upstream library label Jul 28, 2022
@chapulina chapulina enabled auto-merge (squash) July 28, 2022 04:45
@chapulina chapulina merged commit 39bbcdf into main Jul 29, 2022
@chapulina chapulina deleted the chapulina/9/added_mass branch July 29, 2022 16:46
scpeters
scpeters reviewed Jul 29, 2022
View reviewed changes
proto/gz/msgs/inertial.proto
/// 22, 23, 24, 25,
/// 33, 34, 35,
/// 44, 45,
/// 55,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't explicitly say how these values are packed into the repeated double vector. From looking at the test, I believe they are packed from left to right starting with the top row, then working down, like [00, ... 05, 11, ... 15, 22, ... 25, 33, ... 35, 44, 45, 55]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's throwing you off, the spacing? The elements are listed in that order exactly

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like you are labeling the elements according to their place in the matrix, not listing the order in which they are placed in the vector

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like the upper triangle of a matrix to me, not a vector

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you suggest the wording that would work for you? Maybe mentioning that the indices are row-first and instead of organized as follows say ordered as follows? Or something completely different?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like the upper triangle of a matrix to me, not a vector

It's both! That's what I was going for at least...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it looks like a matrix to me and the indices are matrix indices, whereas vector indices would just be integers from [0..20]

 0,  1,  2,  3,  4,  5,
     6,  7,  8,  9, 10,
        11, 12, 13, 14,
            15, 16, 17,
                18, 19,
                    20,

@srmainwaring srmainwaring mentioned this pull request Sep 28, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scpeters scpeters scpeters left review comments

@quarkytale quarkytale quarkytale approved these changes

@jennuine jennuine jennuine approved these changes

@caguero caguero Awaiting requested review from caguero caguero is a code owner

Assignees

@chapulina chapulina

Labels
Breaking change Breaks API, ABI or behavior. Must target unstable version. enhancement New feature or request 🌱 garden Ignition Garden MBARI buoy Sponsored by MBARI buoy sim project: https://github.com/osrf/buoy_sim
Projects
Core development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chapulina @scpeters @jennuine @quarkytale