-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
math.pow: add f16, f80, f128, c_longdouble support #23631
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
base: master
Are you sure you want to change the base?
Conversation
I believe the current thinking is that |
Generalizes math.pow to accept f16, f80, and f128 types.
6f8fe98
to
d2e58a1
Compare
try expect(math.approxEqAbs(f16, pow(f16, 37.45, 3.3), 155736.7160616, epsilon16)); | ||
try expect(math.approxEqAbs(f16, pow(f16, 89.123, 3.3), 2722490.231436, epsilon16)); |
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.
Might be a dumb question, but shouldn't these two result in +inf with the results being way over the maximum value of f16?
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.
Good catch! You're right, the expected result for f16
should be +inf
.
The test likely passed because the large literal 155736.7...
also overflows to +inf
when cast to f16
, causing approxEqAbs
to hit the x == y
fast path.
Maybe it's clearer to test explicitly with == math.inf(f16)
in this overflow case?
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.
Oh, I guess that's true. I've mixed up inf comparison and NaN comparison in my mind, but you're right, that's probably what's happening here. But I agree that explicitly comparing to inf or even using math.isInf would be more clear here. At least in my opinion.
It seems that |
Casting
@Empika1 - Can you point to where you see this pattern? I wasn't able to find anything similar myself. Still wondering if we should If it's only NaN, perhaps the existing NaN handling within |
Removes the compile error restricting math.pow to f32/f64, allowing its use with f16, f80, f128, and c_longdouble.
Adds a compile error when T is comptime_float. The current pow implementation relies on concrete float properties (e.g., bit layout via
@typeInfo(T).float.bits
) which aren't available forcomptime_float
as far as I can tell.There may be other ways to handle this, but this approach at least adds support for the concrete floating point types :)
Partly fixes: #23602