Fix what appears to be a copy/paste redundant code problem. Issue #5733 #5734
Conversation
Fix some casting issues. Looks like quite a bit of double vs float slop in this file.
|
Hi @JTrantow |
src/proc/zero-order.cpp
Outdated
| @@ -103,7 +103,7 @@ namespace librealsense | |||
| return val == 0; | |||
| }), values_ir.end()); | |||
|
|
|||
| if (values_rtd.size() == 0 || values_rtd.size() == 0) | |||
| if ((values_rtd.size() == 0) || (values_ir.size() == 0)) | |||
There was a problem hiding this comment.
Why the extra ()?
Using .empty() instead of the comparison to 0 would be more readable.
There was a problem hiding this comment.
Agreed on using .empty() instead and dropping the "extra ()".
I updated my forked branch. What else do I need to do to update the PR?
src/proc/zero-order.cpp
Outdated
| z2rtd(vertices, rtd.data(), intrinsics, options.baseline); | ||
| double rtd_zo_value; | ||
| z2rtd(vertices, rtd.data(), intrinsics, int(options.baseline)); | ||
| double rtd_zo_value; // \\todo Not sure why try_get_zo_rtd_ir_point_values() returns a double and detect_zero_order() uses a float. Which is best representation? |
There was a problem hiding this comment.
You can probably change the zo_value argument (line 118) to double. It should resolve all the warnings about conversions, and make the explicit conversion in line 155 unnecessary.
There was a problem hiding this comment.
I did this in my forked branch. What's the next step to closing this out?
I'll use the github web page as email replies don't appear to keep the message chain.
There was a problem hiding this comment.
I checked your changes and everything's fine, just please remove this todo comment and we're good :)
There was a problem hiding this comment.
I removed todo comment and an unnecessary float cast. Also changed values.reserve((patch_r + 2ULL) (patch_r + 2ULL)) so the '+' and '' don't give precision warning messages.
Final changes are pushed. So if these are accepted this PR and Issue#5733 can be closed.
|
The "extra ()" come from my background in embedded systems. See
https://barrgroup.com/embedded-systems/books/embedded-c-coding-standard/general-rules/parentheses
The .empty() suggestion is better.
if (values_rtd.empty() || values_ir.empty())
So what do I need to do to amend the pull request?
…On Wed, Jan 29, 2020 at 7:50 AM Eran ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/proc/zero-order.cpp
<#5734 (comment)>
:
> @@ -103,7 +103,7 @@ namespace librealsense
return val == 0;
}), values_ir.end());
- if (values_rtd.size() == 0 || values_rtd.size() == 0)
+ if ((values_rtd.size() == 0) || (values_ir.size() == 0))
Why the extra ()?
Using .empty() instead of the comparison to 0 would be more readable.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5734?email_source=notifications&email_token=AAHDVLIMYYBRDDX4MW7H5SDRAGCR3A5CNFSM4KMZOGC2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCTPGBAA#pullrequestreview-350118016>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHDVLM5GHYDIW3E2WBYQO3RAGCR3ANCNFSM4KMZOGCQ>
.
|
|
I'm used to embedded platforms often with a code size or performance
penalty for doubles. It was always a big issue to pick either float or
double.
A quick review on PC float/double performance indicates either float or
double should be fine.
Class variable types should be chosen carefully so explicit casting is
rarely required. Using Lint and high levels of compiler warnings early on
helps immensely with refining your class variable types. I didn't want to
rock the boat by changing class structure elements.
rs_types uses floats so I thought that was the design choice?
My goal here is to get the 5-600 build warnings (VS2019 -w3) down to single
digits and stay sync'd with the master.
…On Wed, Jan 29, 2020 at 7:58 AM Eran ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/proc/zero-order.cpp
<#5734 (comment)>
:
> @@ -142,15 +144,15 @@ namespace librealsense
const zero_order_options& options, int zo_point_x, int zo_point_y)
{
std::vector<double> rtd(intrinsics.height*intrinsics.width);
- z2rtd(vertices, rtd.data(), intrinsics, options.baseline);
- double rtd_zo_value;
+ z2rtd(vertices, rtd.data(), intrinsics, int(options.baseline));
+ double rtd_zo_value; // \\todo Not sure why try_get_zo_rtd_ir_point_values() returns a double and detect_zero_order() uses a float. Which is best representation?
You can probably change the zo_value argument (line 118) to double. It
should resolve all the warnings about conversions, and make the explicit
conversion in line 155 unnecessary.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5734?email_source=notifications&email_token=AAHDVLMVERC7YJW3ZDYAIRLRAGDQLA5CNFSM4KMZOGC2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCTPHNQQ#pullrequestreview-350123714>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHDVLMLADS4QHMKURLTHXDRAGDQLANCNFSM4KMZOGCQ>
.
|
|
Might be unrelated, but since I lately stumbled across the same question "float" or "double" and just used "float" in the hope, it would be more precise - I was wrong. It turned out, that "float" (especially in C++ realsense environment) renders a 4 byte value. It took me a while to find this as a major difference to my well working python script: C was not precise. I changed all float to double and it worked. |
|
Definitely related to the big picture.
The types, in order of increasing range and precision are float, double,
and long double in the C/C++ languages. <float.h> defines the
range/bits/resolution etc... For my x64 Windows VS compiler long doubles
are identical to doubles. Floats are 64bits, doubles are 128 bits.
#define FLT_EPSILON 1.192092896e-07F // smallest such that
1.0+FLT_EPSILON != 1.0
#define DBL_EPSILON 2.2204460492503131e-016 // smallest such that
1.0+DBL_EPSILON != 1.0
#define LDBL_EPSILON DBL_EPSILON // smallest such that
1.0+LDBL_EPSILON != 1.0
rs_types.h and other headers define many of the rs data structures as
floats.
Depending on what you are calculating, you may need to use doubles. You
need to be especially careful when the error from transcendental functions
(sqrt(), exp(), ln()) can accumulate. It's probably ok to store inputs and
output as rs floats and evaluate the algorithm to determine if it requires
doubles in the calculations.
…On Wed, Jan 29, 2020 at 12:22 PM neilyoung ***@***.***> wrote:
Might be unrelated, but since I lately stumbled across the same question
"float" or "double" and just used "float" in the hope, it would be more
precise - I was wrong. It turned out, that "float" (especially in C++
realsense environment) renders a 4 byte value. It took me a while to find
this as a major difference to my well working python script: C was not
precise. I changed all float to double and it worked.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5734?email_source=notifications&email_token=AAHDVLNHFOB35E3DHKZWVZLRAHCP3A5CNFSM4KMZOGC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKIHK3Q#issuecomment-579892590>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHDVLODJIGDRINMEPFIB3LRAHCP3ANCNFSM4KMZOGCQ>
.
|
Use value_rtd.empty() suggestion instead of (values_rtd.size() == 0). Declare zo_value as double. Use doubles throughout detect_zero_order(). Added intialializers for resolutions_depth and read_baseline.
I agree with the reasoning there but only up to a point. "When in doubt" is the key, and so I prefer readability when it's fairly obvious. If you mix && and ||, I can maybe see the point but otherwise, KISS works best. |
Removed an unnecssary float cast. Use +2ULL in the values.reserve((patch_r + 2UL) *(patch_r + 2UL)) line. This promotes each operand to a unsigned long long which matches the size_t used by reserved() and avoids compiler warnings.
|
@JTrantow I think you closed this by mistake... |
#5733 Change second conditional to test values_ir.size().
Also correct some casting and replace auto with the exact types where appropriate. Why use auto's when the exact type is known at this level?
The coding distinctions between float and double don't seem very tight. Looks like floats would work for most of this.