Make wrap() work with integer types #145
Conversation
These should fail with the current wrap() implementation.
The issue is that 1/range is done as an integer division because 1 is cast to the same type as the other inputs. Then the result is also stored to the same type. This value must be floating point, otherwise the (x - low) * inv_range passed to floor() will be wrong, and floor won't do the right thing. Example: wrap(-4, 0, 10) const int range = 10 - 0 = 10 const int inv_range = 1 / 10 = 0 (integer division) const int num_wraps = floor((-4 - 0) * 0) = floor(0) = 0 return -4 - 10 * 0 = -4 should instead be wrap(-4, 0, 10) range = 10 inv_range = 1/10 = 0.1 num_wraps = floor((-4 - 0) * 0.1) = floor(-0.4) = -1 return -4 - 10 * (-1) = 6 This implementation passes all the exisiting floating point unit tests, but it probably also makes wrap<float> slower since some of the calculations will become double precision.
matrix/helper_functions.hpp
Outdated
| @@ -55,7 +55,7 @@ Type wrap(Type x, Type low, Type high) { | |||
| } | |||
|
|
|||
| const Type range = high - low; | |||
| const Type inv_range = Type(1) / range; // should evaluate at compile time, multiplies below at runtime | |||
| const double inv_range = 1.0 / range; // should evaluate at compile time, multiplies below at runtime | |||
There was a problem hiding this comment.
This could massively increase the cost of wrapping for floaton Cortex-4 chips which don't support double. Could you check maybe with godbolt whether this still eliminates the temporaries for floats with ARM GCC 8? Otherwise we might need a template specialization for integer types.
There was a problem hiding this comment.
Probably true, I'll have a look on godbolt tomorrow.
|
Aight, I got this: https://godbolt.org/z/3YbzvE I'm not super experienced with godbolt so I'm not sure I'm getting the comparison I need. Please let me know! From what I can tell, my version invoked with floats runs 7 instructions more than the original, so it'll be a bit slower but not extremely much so. Do you agree? I still feel that maybe creating some specializations could be a good idea. |
|
I made an example with wrap_pi: https://godbolt.org/z/svKsWf So this will need a specialization for integral types. Also, for integral types I don't think we'd want to do an 'invert and multiply' anyways, converting to floating point and back seems the wrong pattern because you will have imprecision at high values, which is fine for floats but for ints will be unexpected. Better to use the |
|
By the way: I just found out about the And yes, I think a separate implementation for integers is the way to go. I'll make one. |
|
I've split it into two functions now, with some funky business to make sure the right function is invoked for the right data types. I'm using |
|
Unfortunately PX4 doesn't have a lot of the more modern C++ stdlibs available on Nuttx, and <type_traits> is one of them (along with std::array, std::initializer_list etc, all of which should be fine in an embedded system). I think your best bet is to make the default call into an integer-optimized version, and have specializations for float and double, otherwise there are too many integer formats to specialize for. |
|
Got it, sorry for the delay here. I've reworked it now. I put the shared floating point implementation into a sub-namespace "detail" to avoid duplicating it. Is that in line with the style you prefer here? |
|
Thanks for the merge! |
I think wrap() in the helper functions fails in some corner cases when used with integer types.
Should it work with integer types? It's templated in such a way that you can certainly invoke it with ints, but it may not be intended for that. In any case, I have a fix, but I'm pushing some failing integer wrap tests first to see the CI fail.
If wrap() isn't meant to work with integer types, then should probably at least document it in the functions docstring,
or preferably use some clever template magic to make it not compile when Type is deduced to an integer type.
See the commit messages for more detail!