8000 ENH: Extend the functionlty of C++ type `np::Half` by seiko2plus · Pull Request #23298 · numpy/numpy · GitHub
[go: up one dir, main page]

Skip to content

ENH: Extend the functionlty of C++ type np::Half #23298

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

Merged
merged 1 commit into from
Apr 25, 2023

Conversation

seiko2plus
Copy link
Member
@seiko2plus seiko2plus commented Feb 28, 2023
  • TBD add support for Arm native half-precision operations
    revert this support due to massive fp exceptions triggered by testing unite test_half.py,

  • optimize float/double conversions on x86, which requires for now raising the baseline features to f16c at least during the build.

  • optimize float/double conversions on ppc64le, which requires for now raising the baseline features to VSX3 at least during the build.

  • Enable np::Half for npymath

@seiko2plus seiko2plus force-pushed the cpp_half_support branch 5 times, most recently from e96f394 to 943b2ef Compare February 28, 2023 13:07
Comment on lines +55 to +70
/// If there are no hardware optimization available, rounding will always
/// be set to ties to even.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this result in a double round-to-even?
Considering a reduced case of floats with 8, 4, and 2 bit of mantissa;

  • 01011111 -> 01 (direct rounding from 8 to 2 bits)
  • 01011111 -> 0110 -> 10 (rounding via the intermediate 4 bits)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I reverted the change and reinstated the previous behavior, which relies on explicit conversion instead of using single-precision as an argument when hardware-native conversion is available

  - optimize float/double conversions on x86, requires for now
    raising up the baseline features to `f16c` at least
    during the build.
  - optimize float/double conversions on ppc64le, requires for now
    raising up the baseline features to `VSX3` at least
    during the build.
  - Brings `np::Half` to npymath
@seiko2plus seiko2plus marked this pull request as ready for review April 5, 2023 07:15
@seiko2plus
Copy link
Member Author

close/reopen CI errors seems unrelated

8000 Copy link
Member
@seberg seberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't want to nitpick grammar here and it is mostly a reorganization (thanks for spotting that subtle rounding issue Eric! I am very sure we have tests to spot such an error luckily.).

Thanks Sayed, lets get this in and unblock the ufunc work!

EDIT: In case anyone wonders, the exported symbols stay identical (except that the private namespace is added), so that is fine.


/// @name Comparison with no guarantee of NaN behavior
/// @{
constexpr bool Less(Half r) const
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fair, I wonder whether Less and LessEqual is a typical nomenclature for no-nan versions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its fine since comparison operators are defined to handles NaNs. using methods Less or LessEqual instead of comparison operators and also instead of std::less or std::less_equal suppose to infuriates any C++ coder for the reason.

@seberg seberg merged commit 6f3e1f4 into numpy:main Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0