Skip to content

Uniformize Interface of Random Generators in Math #12410

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Uniformize Interface of Random Generators in Math #12410

wants to merge 3 commits into from

Conversation

VinInn
Copy link

@VinInn VinInn commented Mar 2, 2023

Changes or fixes:

added/modified MinInt, MaxInt, IntRndm.
added test of Integer interface

Checklist:

  • [ x] tested changes locally
  • updated the docs (if necessary)

@VinInn VinInn requested a review from lmoneta as a code owner March 2, 2023 10:24
@phsft-bot
Copy link

Can one of the admins verify this patch?

@lmoneta lmoneta self-assigned this Mar 2, 2023
@hahnjo hahnjo self-requested a review March 8, 2023 09:41
@hahnjo
Copy link
Member

hahnjo commented Mar 8, 2023

@phsft-bot build

@phsft-bot
Copy link

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac12/noimt, mac11/cxx14, windows10/cxx14
How to customize builds

@VinInn
Copy link
Author

VinInn commented Mar 8, 2023

DO not understand clang-format failure.
ubuntu20 failure seems unrelated

@phsft-bot
Copy link

Build failed on mac12/noimt.
Running on macphsft18.dyndns.cern.ch:/Users/sftnight/build/jenkins/workspace/root-pullrequests-build
See console output.

Warnings:

  • [2023-03-08T09:50:36.671Z] /Users/sftnight/build/jenkins/workspace/root-pullrequests-build/root/math/mathcore/test/testMathRandom.cxx:43:77: warning: 'static_assert' with no message is a C++17 extension [-Wc++17-extensions]
  • [2023-03-08T09:50:36.671Z] /Users/sftnight/build/jenkins/workspace/root-pullrequests-build/root/math/mathcore/test/testMathRandom.cxx:44:39: warning: 'static_assert' with no message is a C++17 extension [-Wc++17-extensions]
  • [2023-03-08T09:50:36.671Z] /Users/sftnight/build/jenkins/workspace/root-pullrequests-build/root/math/mathcore/test/testMathRandom.cxx:45:81: warning: 'static_assert' with no message is a C++17 extension [-Wc++17-extensions]

@VinInn
Copy link
Author

VinInn commented Mar 8, 2023

did the mac build fail because of the warnings?

@phsft-bot
Copy link

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Errors:

  • [2023-03-08T10:48:32.581Z] C:\build\workspace\root-pullrequests-build\root\math\mathcore\test\testMathRandom.cxx(45,48): error C3861: '__builtin_clzll': identifier not found [C:\build\workspace\root-pullrequests-build\build\math\mathcore\test\testMathRandom.vcxproj]

@phsft-bot
Copy link

Build failed on ROOT-debian10-i386/soversion.
Running on pcepsft11.dyndns.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Warnings:

  • [2023-03-08T09:58:05.891Z] /home/sftnight/build/workspace/root-pullrequests-build/root/math/mathmore/src/GSLRngROOTWrapper.h:79:57: warning: conversion from ‘long long unsigned int’ to ‘long unsigned int’ changes value from ‘2305843009213693951’ to ‘4294967295’ [-Woverflow]

@hahnjo
Copy link
Member

hahnjo commented Mar 8, 2023

DO not understand clang-format failure.

My 2 cents: clang-format is unhappy about some old code, but I think it's not a good idea to fix it in the same commit that also introduces functional changes. On the contrary, it makes reviewing the diff quite difficult...

hahnjo
hahnjo reviewed Mar 8, 2023
View reviewed changes
math/mathcore/inc/Math/StdEngine.h
Comment on lines +98 to +102
#ifdef _WIN64
static constexpr uint64_t kNumberOfBits = _popcount64(Generator::max());
#else
static constexpr uint64_t kNumberOfBits = __builtin_popcountll(Generator::max());
#endif
Copy link
Member

Choose a reason for hiding this comment

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

I'm not particularly happy about using builtins in the header. Is there no other way to get this information from the C++ type?

Copy link
Author

Choose a reason for hiding this comment

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

in c++20 there is a std:;popcount.
There are generators that return 48 or 61 bits. so it is not just numeric_limits<Return_type>::bits()

p.s. I agree that mixing clang-format and changes is not ideal and I do try to avoid it.
I did not expect it to make so many changes.

Copy link
Author

Choose a reason for hiding this comment

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

We may argue that kNumberOfBits is redundant and could be defined in the user code (if needed)

Copy link
Member

Choose a reason for hiding this comment

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

Fine with me. Also, there may be generators that don't have a max() that falls on a power-of-two (LCGs come to mind)

math/mathcore/test/testMathRandom.cxx
Comment on lines +46 to +53
#ifdef _WIN64
static_assert(Engine::kNumberOfBits == _popcount64(Engine::MaxInt()), "MaxInt inconsistent with kNumberOfBits");
#else
static_assert(Engine::kNumberOfBits == __builtin_popcountll(Engine::MaxInt()),
"MaxInt inconsistent with kNumberOfBits");
static_assert(64 - Engine::kNumberOfBits == __builtin_clzll(Engine::MaxInt()),
"MaxInt inconsistent with kNumberOfBits");
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Do these tests provide additional coverage? You're already testing above that (~0ULL) >> (64 - Engine::kNumberOfBits) == Engine::MaxInt(). Counting that there are exactly Engine::kNumberOfBits set is a weaker check, no?

Copy link
Author

@VinInn VinInn Mar 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

just verify that the expression is indeed constexpr.

@phsft-bot
Copy link

Build failed on mac11/cxx14.
Running on macphsft20.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Warnings:

  • [2023-03-08T11:39:45.483Z] /Users/sftnight/build/workspace/root-pullrequests-build/root/math/mathcore/test/testMathRandom.cxx:43:77: warning: static_assert with no message is a C++17 extension [-Wc++17-extensions]
  • [2023-03-08T11:39:45.483Z] /Users/sftnight/build/workspace/root-pullrequests-build/root/math/mathcore/test/testMathRandom.cxx:44:39: warning: static_assert with no message is a C++17 extension [-Wc++17-extensions]
  • [2023-03-08T11:39:45.483Z] /Users/sftnight/build/workspace/root-pullrequests-build/root/math/mathcore/test/testMathRandom.cxx:45:81: warning: static_assert with no message is a C++17 extension [-Wc++17-extensions]

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

@hahnjo hahnjo hahnjo left review comments

@lmoneta lmoneta Awaiting requested review from lmoneta lmoneta is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@lmoneta lmoneta

Labels
in:Math Libraries
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@VinInn @phsft-bot @hahnjo @lmoneta @guitargeek