Skip to content

👩‍🌾 Prevent -0 with out stream operator#206

Merged
chapulina merged 3 commits intomainfrom
chapulina/6/-0
Jun 24, 2021
Merged

👩‍🌾 Prevent -0 with out stream operator#206
chapulina merged 3 commits intomainfrom
chapulina/6/-0

Conversation

@chapulina
Copy link
Copy Markdown
Contributor

@chapulina chapulina commented Apr 19, 2021

🦟 Bug fix

Fixes #161

Summary

Implemented @azeey 's suggestion to check for zeroes and explicitly output 0 to avoid -0.

I also had to add a const version of Color::operator[] so it could be used by the operator.

Checklist

  • Signed all commits for DCO
  • Added tests (all existing tests should still pass)
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • 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

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

https://github.com/osrf/buildfarmer/issues/181

@chapulina chapulina requested a review from scpeters as a code owner April 19, 2021 22:24
@github-actions github-actions bot added 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels Apr 19, 2021
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 19, 2021 •
edited
Loading

Codecov Report

Merging #206 (b87c45b) into main (efb5db3) will increase coverage by 0.67%.
The diff coverage is 100.00%.

❗ Current head b87c45b differs from pull request most recent head b007963. Consider uploading reports for the commit b007963 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #206      +/-   ##
==========================================
+ Coverage   98.53%   99.21%   +0.67%     
==========================================
  Files          61       65       +4     
  Lines        6001     6094      +93     
==========================================
+ Hits         5913     6046     +133     
+ Misses         88       48      -40
Impacted Files Coverage Δ
include/ignition/math/Angle.hh 100.00% <ø> (ø)
include/ignition/math/Box.hh 100.00% <ø> (ø)
include/ignition/math/MassMatrix3.hh 99.40% <ø> (ø)
src/Angle.cc 100.00% <ø> (+10.34%) ⬆️
include/ignition/math/Color.hh 100.00% <100.00%> (ø)
include/ignition/math/Ellipsoid.hh 100.00% <100.00%> (ø)
include/ignition/math/Helpers.hh 98.55% <100.00%> (+0.05%) ⬆️
include/ignition/math/Matrix3.hh 100.00% <100.00%> (ø)
include/ignition/math/Matrix4.hh 100.00% <100.00%> (ø)
include/ignition/math/Quaternion.hh 100.00% <100.00%> (+4.10%) ⬆️
... and 25 more

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 efb5db3...b007963. Read the comment docs.

@chapulina chapulina mentioned this pull request Apr 27, 2021
Closed
scpeters
scpeters reviewed May 1, 2021
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 added a test case in e3c7b81 that fails on ign-math6: printing a Pose3::Zero object results in 0 0 0 0 -0 0. With this branch we can expect all 0 values

there are some behavior changes by setting precision to 6 for all our output streams, though it does make them more consistent

@chapulina
Copy link
Copy Markdown
Contributor Author

chapulina commented May 3, 2021

I added a test case in e3c7b81 that fails on ign-math6

Thank you for the test case. I'm on he fence here about whether it's a good idea to get this PR in. Consistency is good, but behavior change, even if it's almost harmless, can be disruptive for users who have been handling the current behaviour. I can target this at main if you prefer.

scpeters
scpeters reviewed May 6, 2021
View reviewed changes
@azeey
Copy link
Copy Markdown
Contributor

azeey commented May 11, 2021

I just want to point out that the use of std::fpclassify in libsdformat is limited to the URDF parser within libsdformat (https://github.com/osrf/sdformat/blob/abc6615d4ad3dc22a2756a2fbcbcb7e8662c2fb5/src/parser_urdf.cc#L1113). libsdformat still relies on the operator<< of ignition::math::Pose3d when generating SDFormat output strings. There are tests in libsdformat that expect -0 to be output. I am okay with updating them to 0 immediately after this is merged, but I just wanted to point out the behavior change.

@chapulina
Copy link
Copy Markdown
Contributor Author

chapulina commented May 11, 2021

There are tests in libsdformat that expect -0 to be output. I am okay with updating them to 0 immediately after this is merged, but I just wanted to point out the behavior change.

Ouch that's good to know. I think we should retarget this to main. Ignition Math 6 is used across various projects and has been around for over 2 years. I think it's safe to assume that this will break users while bringing little benefit.

@chapulina
Prevent -0 with out stream operator
ed58a07 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@chapulina chapulina changed the base branch from ign-math6 to main May 14, 2021 23:57
@chapulina chapulina removed 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels May 14, 2021
@chapulina
changelog, migration
4c946a1 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Copy link
Copy Markdown
Contributor Author

chapulina commented May 15, 2021

humm I don't know what's happening on CI:

stderr: error: unable to create file tools/check_test_ran.py (Permission denied)
error: unable to create file tools/code_check.sh (Permission denied)
fatal: cannot create directory at 'tools/cppcheck_rules': Permission denied

Seems related to #211. Any ideas, @j-rivero ?

@scpeters scpeters requested a review from j-rivero May 24, 2021 18:30
@j-rivero
Copy link
Copy Markdown
Contributor

j-rivero commented May 27, 2021

Seems related to #211. Any ideas, @j-rivero ?

For some reason the tools/ directory was under root permissions (could be a build finished unsuccessful). I've change it to jenkins and things are working as expected.

j-rivero
j-rivero reviewed May 28, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@j-rivero j-rivero left a comment

Choose a reason for hiding this comment

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

Minor suggestion for the Migration document. Looks good to me. I was trying to measure the impact on performance of changes by playing with time command but my results are pretty inconsistent. Given that it will only affect to the output I'm not that worried.

@chapulina
update migration
b007963 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Copy link
Copy Markdown
Contributor Author

chapulina commented Jun 14, 2021

I was trying to measure the impact on performance of changes by playing with time command but my results are pretty inconsistent. Given that it will only affect to the output I'm not that worried.

That's a good idea. This may actually impact Gazebo, because the stream operators are used for serialization, and that happens often. So let's keep an eye on that 👁️

@chapulina chapulina enabled auto-merge (squash) June 14, 2021 20:58
@chapulina chapulina merged commit 12f5f7f into main Jun 24, 2021
@chapulina chapulina deleted the chapulina/6/-0 branch June 24, 2021 16:06
@scpeters scpeters mentioned this pull request Aug 19, 2021
Closed
@aaronchongth aaronchongth mentioned this pull request Dec 18, 2021
Merged
8 tasks
@scpeters scpeters mentioned this pull request Dec 27, 2021
Merged
scpeters added a commit to gazebosim/sdformat that referenced this pull request Dec 29, 2021
@scpeters
Expext 0 instead of -0 in tests
3ab919e 
Caused by gazebosim/gz-math#206

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
scpeters added a commit to gazebosim/sdformat that referenced this pull request Dec 29, 2021
@chapulina @scpeters
Bumps in garden: use ign-math7 (#803)
1495b3f 
* Address ign-math7 deprecations

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Expext 0 instead of -0 in tests
  Caused by gazebosim/gz-math#206
* macos workflow: try to checkout ci_matching_branch
* macOS workflow: remove swig to fix ignition-math7 build

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Co-authored-by: Steve Peters <scpeters@openrobotics.org>
This was referenced Jul 6, 2022
Merged
Merged
Merged
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

@j-rivero j-rivero j-rivero approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

arm64 test failures during debbuild

5 participants

@chapulina @azeey @j-rivero @scpeters @osrf-triage