Add pure FixedPoint implementations and tests for sin(), cos(), and tan() #1773
Conversation
| return this->get_integral_part(); | ||
| } | ||
| else { | ||
| return this->get_integral_part() + one(); |
There was a problem hiding this comment.
| return this->get_integral_part() + one(); | |
| return this->get_integral_part() + FixedPoint::one(); |
| /** | ||
| * Converter to retrieve the integral (pre-decimal) part of the number. | ||
| */ | ||
| constexpr FixedPoint get_integral_part() const { |
There was a problem hiding this comment.
Can you add tests for this method?
There was a problem hiding this comment.
Yep! I'll try to get those added soon!
| /** | ||
| * Round to the nearest integer. | ||
| */ | ||
| constexpr FixedPoint round() const { |
There was a problem hiding this comment.
Can you add tests for this method?
| if (this->get_fractional_part() < same_type_but_unsigned(0.5)) { | ||
| return this->get_integral_part(); | ||
| } |
There was a problem hiding this comment.
We should probably consider IEE 754 rounding rules here (see Wikipedia for details). The default one seems to be "round to nearest, ties to even". That would mean there needs to be an additional condition for when the fraction is exactly == 0.5. The rules are a bit counter-intuitive because rounding in real-life is done differently, but apparently it checks out when I test this with Python:
-1.5=>-2-0.5=>00.5=>01.5=>22.5=>2
There was a problem hiding this comment.
You should also mention the rounding rule in the docstring.
There was a problem hiding this comment.
Good call, I'll take a look soon!
| // special case when integeral part of rhs is 0 | ||
| if (rhs_upper == 0) { | ||
| // It's very likely this upper term is zero, but we should consider it regardless. | ||
| FP upper_term = FP::from_raw_value((lhs_upper << (2 * F)) / rhs_lower); |
There was a problem hiding this comment.
This generates compiler warning because you are shifting by 2 * 32 sometimes when lhs_upper is a 64-Bit signed integer which causes undefined behavior. You should consider using safe_shiftleft.
| FP upper_term = FixedPoint<I, F, Inter>::from_raw_value((lhs_upper << F) / rhs_upper); | ||
| FP lower_term = FP::from_raw_value(((lhs_lower << F) / rhs_upper) >> F); | ||
| FP mixed_term = FixedPoint<I, F, Inter>::from_raw_value((rhs_lower) / rhs_upper); |
There was a problem hiding this comment.
| FP upper_term = FixedPoint<I, F, Inter>::from_raw_value((lhs_upper << F) / rhs_upper); | |
| FP lower_term = FP::from_raw_value(((lhs_lower << F) / rhs_upper) >> F); | |
| FP mixed_term = FixedPoint<I, F, Inter>::from_raw_value((rhs_lower) / rhs_upper); | |
| FP upper_term = FP::from_raw_value((lhs_upper << F) / rhs_upper); | |
| FP lower_term = FP::from_raw_value(((lhs_lower << F) / rhs_upper) >> F); | |
| FP mixed_term = FP::from_raw_value((rhs_lower) / rhs_upper); |
| @@ -127,7 +127,8 @@ void fixed_point() { | |||
| using T = FixedPoint<uint16_t, 7U, uint64_t>; | |||
|
|
|||
| auto a = S::from_int(16U); | |||
| TESTNOTEQUALS((a*a).to_int(), 256U); | |||
| // This test case is now equal with the improved multiplication algorithm | |||
There was a problem hiding this comment.
You should probably remove the comment since no one will know what "improved multiplication algorithm" refers to in a few months :D
| @@ -139,7 +140,8 @@ void fixed_point() { | |||
| using S = FixedPoint<int32_t, 12U>; | |||
| auto a = S::from_int(256); | |||
| auto b = S::from_int(8); | |||
| TESTNOTEQUALS((a/b).to_int(), 32); | |||
| // This test case is now equal with the improved division algorithm | |||
| using TrigType = FixedPoint<int64_t, 32, int64_t>; | ||
|
|
||
| // Testing sin() and cos() | ||
| for (int i = -100'000; i <= 100'000; i++) { |
There was a problem hiding this comment.
the loop runs for a very long time. Can we go with lower precision for speeding these up a bit?
heinezen
added
improvement
Per #1543.
Notes:
This uses a power series approximation to calculate sine and cosine. Tangent is just calculated using
sin()andcos(). The input value is first shifted into the interval(-pi, pi)forsin()andcos()and(-pi/2, pi/2)fortan(). At the asymptotes for tan, an exception will be thrown because the FixedPoint doesn't implementinf(yet). Near the asymptotes absolute precision is a bit poor.It's possible to use another expansion for
tan(), but it's not as nice as those forsin()andcos(). I'd be happy to implement that down the road if that would be preferred. (It will still have problems at the asymptotes though).