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

7 to main #540

Merged
merged 5 commits into from
Aug 2, 2023
Merged

7 to main #540

merged 5 commits into from
Aug 2, 2023

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Jul 27, 2023

➡️ Forward port

Port 7 to main

Branch comparison: main...gz-math7

Note to maintainers: Remember to Merge with commit (not squash-merge or rebase)

azeey and others added 4 commits June 14, 2023 23:19
@azeey
Release 7.2.0 (#536)
2267187 
Signed-off-by: Addisu Z. Taddese <[email protected]>
@jasmeet0915
Added std::optional MassMatrix() functions for Box, Cylinder & Sphe…
cf5d7f3 
…re (#538)

* Added std::optional MassMatrix function to box, sphere & cylinder

Signed-off-by: Jasmeet Singh <[email protected]>

* Used py::overload_cast with py::const_ for MassMatrix functions in py bindings

Signed-off-by: Jasmeet Singh <[email protected]>

* Included <optional> in Cylinder, Sphere & Box headers

Signed-off-by: Jasmeet Singh <[email protected]>

* Added unit tests for std::optional MassMatrix() for box, cylinder & sphere

Signed-off-by: Jasmeet Singh <[email protected]>

* Added python tests for new MassMatrix() function for Box, sphere & Cylinder

Signed-off-by: Jasmeet Singh <[email protected]>

* Included <optional> in detail Cylinder, Sphere & Box headers

Signed-off-by: Jasmeet Singh <[email protected]>

---------

Signed-off-by: Jasmeet Singh <[email protected]>
@mjcarroll
Build examples from CMake rather than executable (#482)
f64cb8c 
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
@ahcorde
Merge remote-tracking branch 'origin/gz-math7' into ahcorde/7_to_main…
8a5bb56 
…/270723
@github-actions github-actions bot added 🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic labels Jul 27, 2023
@ahcorde ahcorde mentioned this pull request Jul 27, 2023
Merged
9 tasks
@codecov
Copy link

codecov bot commented Jul 27, 2023 •
edited
Loading

Codecov Report

Merging #540 (746237f) into main (92d6063) will decrease coverage by 5.21%.
The diff coverage is 90.90%.

❗ Current head 746237f differs from pull request most recent head 11bdf53. Consider uploading reports for the commit 11bdf53 to get more accurate results

@@            Coverage Diff             @@
##             main     #540      +/-   ##
==========================================
- Coverage   99.74%   94.53%   -5.21%     
==========================================
  Files          77      144      +67     
  Lines        7032     9626    +2594     
==========================================
+ Hits         7014     9100    +2086     
- Misses         18      526     +508
Files Changed Coverage Δ
include/gz/math/Box.hh 100.00% <ø> (ø)
include/gz/math/Cylinder.hh 100.00% <ø> (ø)
include/gz/math/Sphere.hh 100.00% <ø> (ø)
include/gz/math/detail/Box.hh 98.02% <80.00%> (-1.98%) ⬇️
include/gz/math/detail/Sphere.hh 98.33% <80.00%> (-1.67%) ⬇️
include/gz/math/detail/Cylinder.hh 98.30% <85.71%> (-1.70%) ⬇️
include/gz/math/Helpers.hh 100.00% <100.00%> (ø)
include/gz/math/Polynomial3.hh 100.00% <100.00%> (ø)
src/python_pybind11/src/Box.hh 95.83% <100.00%> (ø)
src/python_pybind11/src/Cylinder.hh 82.35% <100.00%> (ø)
... and 2 more

... and 77 files with indirect coverage changes

@azeey
Copy link
Contributor

azeey commented Jul 28, 2023

This is currently targetting gz-math7. Should it target main?

@ahcorde ahcorde changed the base branch from gz-math7 to main July 28, 2023 06:50
@ahcorde
Copy link
Contributor Author

ahcorde commented Jul 28, 2023

This is currently targetting gz-math7. Should it target main?

Done

@azeey azeey added beta Targeting beta release of upcoming collection and removed 🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic labels Jul 31, 2023
adityapande-1995
adityapande-1995 reviewed Jul 31, 2023
View reviewed changes
Copy link
Contributor

@adityapande-1995 adityapande-1995 left a comment

Choose a reason for hiding this comment

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

Could you add small tests for the newly added functions to make codecov happy ?
Not sure why ABI checker is failing here.

@ahcorde
TODOs in main (#541)
11bdf53 
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@azeey
Copy link
Contributor

azeey commented Aug 1, 2023

Could you add small tests for the newly added functions to make codecov happy ? Not sure why ABI checker is failing here.

The ABI checker is failing because this PR was originally targeting gz-math7. Now that it's targetting main. we can ignore the ABI checker (it's okay to break ABI for main).
Also, since this is a forward port of changes in gz-math7, we generally don't add new tests. It should be sufficient to check that the changes from the older branch are compatible with the new branch. If there's a particular concern there, maybe a test would be warranted.

@mjcarroll
Copy link
Contributor

I think this one is good to go. ABI checker is a non-issue and we can increase coverage in a later PR, as this is just a forward port.

@mjcarroll mjcarroll merged commit b7d2a60 into main Aug 2, 2023
6 of 8 checks passed
@mjcarroll mjcarroll deleted the ahcorde/7_to_main/270723 branch August 2, 2023 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adityapande-1995 adityapande-1995 adityapande-1995 approved these changes

@mjcarroll mjcarroll mjcarroll approved these changes

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

Assignees
No one assigned
Labels
beta Targeting beta release of upcoming collection
Projects
Core development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ahcorde @azeey @mjcarroll @Blast545 @adityapande-1995 @jasmeet0915