GH-49123: [C++] Complete std::numeric_limits<Float16> specialization#49124
Open
HyukjinKwon wants to merge 1 commit intoapache:mainfrom
Open
GH-49123: [C++] Complete std::numeric_limits<Float16> specialization#49124HyukjinKwon wants to merge 1 commit intoapache:mainfrom
HyukjinKwon wants to merge 1 commit intoapache:mainfrom
Conversation
|
|
pitrou
requested changes
Feb 4, 2026
Member
pitrou
left a comment
pitrou
left a comment
There was a problem hiding this comment.
Thanks for doing this. I think this looks good, here are just a couple potential additions.
| ASSERT_TRUE(F16::denorm_min() > Float16::FromBits(0)); | ||
| ASSERT_TRUE(F16::denorm_min() < F16::min()); | ||
|
|
||
| // Verify epsilon: 1 + epsilon != 1 |
Member
There was a problem hiding this comment.
We may also verify that 1 + (epsilon / 2) == 1
| ASSERT_TRUE((-F16::infinity()).is_infinity()); | ||
| ASSERT_TRUE(F16::min() > Float16::FromBits(0)); | ||
| ASSERT_TRUE(F16::denorm_min() > Float16::FromBits(0)); | ||
| ASSERT_TRUE(F16::denorm_min() < F16::min()); |
Member
There was a problem hiding this comment.
Perhaps also verify that denorm_min / 2 == 0? Am I assuming right?
| ASSERT_EQ(F16::max_exponent10, 4); | ||
|
|
||
| // Special values | ||
| ASSERT_FLOAT_EQ(F16::max().ToFloat(), 65504.0f); // Largest finite value |
Member
There was a problem hiding this comment.
I wonder if we can also add this:
Suggested change
| ASSERT_FLOAT_EQ(F16::max().ToFloat(), 65504.0f); // Largest finite value | |
| ASSERT_FLOAT_EQ(F16::max().ToFloat(), 65504.0f); // Largest finite value | |
| ASSERT_TRUE(Float16::FromBits(F16::max().bits() + 1U).is_infinity()); |
| static constexpr bool has_infinity = true; | ||
| static constexpr bool has_quiet_NaN = true; | ||
| static constexpr bool has_signaling_NaN = true; | ||
| static constexpr std::float_denorm_style has_denorm = std::denorm_present; |
Member
There was a problem hiding this comment.
This seems to be deprecated in C++23, so perhaps we shouldn't bother?
github-actions
bot
added
awaiting committer review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
The
std::numeric_limits<Float16>specialization was incomplete, missing members.What changes are included in this PR?
std::numeric_limits<Float16>with missing members (type traits, precision/exponent values, special value functions)Are these changes tested?
Added
Float16Test.NumericLimitsvalidating all members against IEEE 754 semantics.Are there any user-facing changes?
Yes. Users can now use all
std::numeric_limits<Float16>members in generic template code.