-
Notifications
You must be signed in to change notification settings - Fork 95
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
Changed default floating point precision to max #872
Conversation
Signed-off-by: Jenn Nguyen <[email protected]>
Signed-off-by: Jenn Nguyen <[email protected]>
Signed-off-by: Jenn Nguyen <[email protected]>
Signed-off-by: Jenn Nguyen <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #872 +/- ##
==========================================
+ Coverage 88.14% 88.23% +0.09%
==========================================
Files 102 102
Lines 14706 14693 -13
==========================================
+ Hits 12962 12964 +2
+ Misses 1744 1729 -15
Continue to review full report at Codecov.
|
I see some test failures on macOS and windows in
|
hmm, so the values for macOS and windows were the values I was expecting but the values in linux were off by about ~2e-16. In a VC with @azeey we believed it was related to gazebosim/gz-math#212 since this loss doesn't happen with I'm surprised that this loss doesn't happen in macOS or windows.. Instead of doing a direct comparison of |
Per VC, maybe related to |
Signed-off-by: Jenn Nguyen <[email protected]>
Per VC, suggestions:
|
Signed-off-by: Jenn Nguyen <[email protected]>
Signed-off-by: Jenn Nguyen <[email protected]>
Signed-off-by: Jenn Nguyen <[email protected]>
d19c1fd
to
fa8e4a2
Compare
The precision issue came from ign-math's The workaround is described below.
Now that CI is green, this PR is ready for review. |
@@ -75,38 +75,48 @@ namespace sdf | |||
struct ParamStreamer | |||
{ | |||
const T &val; | |||
const int precision; // Used to set std::ostream's std::setprecision |
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 suggest we use a constructor for ParamStreamer
instead of the template guides below. This would ensure that precision is initialized to zero.
ParamStreamer(const T &_val, int _precision = 0)
: val(_val), precision(_precision) {}
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.
Thanks for the suggestion 855cb1d
include/sdf/Param.hh
Outdated
}; | ||
|
||
template<class T> ParamStreamer(T) -> ParamStreamer<T>; | ||
template<class T> ParamStreamer(T, bool) -> ParamStreamer<T>; |
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.
These template guides can be removed if we create a constructor per my comment above.
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.
src/cmd/cmdsdformat.rb.in
Outdated
@@ -42,14 +42,16 @@ COMMANDS = { 'sdf' => | |||
" -d [ --describe ] [SPEC VERSION] Print the aggregated SDFormat spec description. Default version (@SDF_PROTOCOL_VERSION@).\n" + | |||
" -g [ --graph ] <pose, frame> arg Print the PoseRelativeTo or FrameAttachedTo graph. (WARNING: This is for advanced\n" + | |||
" use only and the output may change without any promise of stability)\n" + | |||
" -p [ --print ] arg Print converted arg.\n" + | |||
" -p [ --print ] arg Print converted arg. Note poses and unit vectors will be normalized.\n" + |
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.
It's not clear what is meant by normalization of Poses. It's the quaternion representation of the rotational part of the pose that gets normalized.
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.
src/ign.cc
Outdated
@@ -134,7 +134,8 @@ extern "C" SDFORMAT_VISIBLE int cmdDescribe(const char *_version) | |||
|
|||
////////////////////////////////////////////////// | |||
extern "C" SDFORMAT_VISIBLE int cmdPrint(const char *_path, | |||
int inDegrees, int snapToDegrees, float snapTolerance) | |||
int _inDegrees, int _snapToDegrees, float _snapTolerance, | |||
bool _preserveIncludes, int _outPrecision) |
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'm not sure if it's safe to use bool
in the function signature of an extern "C"
function since bool
is not builtin type in C
. The parameter _inDegrees
is technically a boolean, but we're using int
for this reason, I think.
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.
src/cmd/cmdsdformat.rb.in
Outdated
Importer.extern 'int cmdPrintPreserveIncludes(const char *)' | ||
exit(Importer.cmdPrintPreserveIncludes(File.expand_path(options['print']))) | ||
elsif options.key?('snap_to_degrees') | ||
precision = -9999 |
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 assume this is a placeholder number for an invalid precision. Usually I see -1
used for that purpose, so seeing -9999
was a little confusing.
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.
Initially, I set it as -9999
since technically negative numbers in setprecision
are allowed but silently ignored. I agree that this was a little confusing so I modified it to 0
and updated the -h
docs to state the argument must be a positive integer: 855cb1d
Signed-off-by: Jenn Nguyen <[email protected]>
Signed-off-by: Jenn Nguyen <[email protected]>
4e5ebff
to
855cb1d
Compare
@osrf-jenkins run tests please |
🎉 New feature
Closes #790
Summary
Previously, there were different precisions as discussed here: #689 (review)
This PR changes the behavior so that all floating types emitted to the output stream are maximum precision by default for consistency. A new
PrintConfig::SetOutPrecision
option was added so that the precision can be configured as desired.Test it
or with the command line tool
test.sdf
This IEEE 754 floating point converter can be used to determine expected strings: https://baseconvert.com/ieee-754-floating-point
double
(64 bit) has a max of 17 digits andfloat
(32 bit) has a max of 9 digits.There may be a small loss of precision (~2e-16), which is related to gazebosim/gz-math#212
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.