Skip to content

Comments

GH-48123 [C++][Float16] Reimplement arrow::WithinUlp and Enable it for float16#48224

Merged
pitrou merged 3 commits intoapache:mainfrom
andishgar:ulp_distance
Dec 8, 2025
Merged

GH-48123 [C++][Float16] Reimplement arrow::WithinUlp and Enable it for float16#48224
pitrou merged 3 commits intoapache:mainfrom
andishgar:ulp_distance

Conversation

@andishgar
Copy link
Contributor

@andishgar andishgar commented Nov 22, 2025

Rationale for this change

Refer to this comment. Additionally, this change enables arrow::WithinUlp for float16.

What changes are included in this PR?

Re-implement arrow::WithinUlp and enable it for float16, including relevant tests for corner cases around powers of two and Float16.

Are these changes tested?

Yes, I ran the relevant unit tests.

Are there any user-facing changes?

No.

@github-actions
Copy link

⚠️ GitHub issue #48123 has been automatically assigned in GitHub to PR creator.

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

This is neat, thank you. A couple comments below, but looks good on the principle.

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Nov 26, 2025
@andishgar
Copy link
Contributor Author

@pitrou Thank you for the review. I’ve checked all the comments and will apply your suggestions in the coming days.

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @andishgar ! I've just pushed a very minor change and will merge if CI is ok.

@pitrou
Copy link
Member

pitrou commented Dec 8, 2025

@github-actions crossbow submit -g cpp

@pitrou pitrou added the CI: Extra: C++ Run extra C++ CI label Dec 8, 2025
@github-actions
Copy link

github-actions bot commented Dec 8, 2025

Revision: 0597736

Submitted crossbow builds: ursacomputing/crossbow @ actions-aeed0cd233

Task Status
example-cpp-minimal-build-static GitHub Actions
example-cpp-minimal-build-static-system-dependency GitHub Actions
example-cpp-tutorial GitHub Actions
test-build-cpp-fuzz GitHub Actions
test-conda-cpp GitHub Actions
test-conda-cpp-valgrind GitHub Actions
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1 GitHub Actions
test-cuda-cpp-ubuntu-24.04-cuda-13.0.2 GitHub Actions
test-debian-12-cpp-amd64 GitHub Actions
test-debian-12-cpp-i386 GitHub Actions
test-fedora-42-cpp GitHub Actions
test-ubuntu-22.04-cpp GitHub Actions
test-ubuntu-22.04-cpp-20 GitHub Actions
test-ubuntu-22.04-cpp-bundled GitHub Actions
test-ubuntu-22.04-cpp-emscripten GitHub Actions
test-ubuntu-22.04-cpp-no-threading GitHub Actions
test-ubuntu-24.04-cpp GitHub Actions
test-ubuntu-24.04-cpp-bundled-offline GitHub Actions
test-ubuntu-24.04-cpp-gcc-13-bundled GitHub Actions
test-ubuntu-24.04-cpp-gcc-14 GitHub Actions
test-ubuntu-24.04-cpp-minimal-with-formats GitHub Actions
test-ubuntu-24.04-cpp-thread-sanitizer GitHub Actions

@pitrou pitrou merged commit f40d62e into apache:main Dec 8, 2025
56 of 57 checks passed
@pitrou pitrou removed the awaiting committer review Awaiting committer review label Dec 8, 2025
@andishgar andishgar deleted the ulp_distance branch December 8, 2025 13:42
@conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit f40d62e.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 16 possible false positives for unstable benchmarks that are known to sometimes produce them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants