Skip to content

Removed skipping of 0 (zero) in TRandom3.Rndm() #14702

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 8 commits into
base: master
Choose a base branch
from
Open

Removed skipping of 0 (zero) in TRandom3.Rndm() #14702

wants to merge 8 commits into from

Conversation

kjvbrt
Copy link
Contributor

@kjvbrt kjvbrt commented Feb 13, 2024

This Pull request: Removes skipping of 0 (zero) in TRandom3.Rndm()

Changes or fixes:

  • Removes skipping of 0 (zero) in TRandom3.Rndm()
  • Updates documentation, adds warnings

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

This PR fixes #14581

@kjvbrt kjvbrt requested a review from lmoneta as a code owner February 13, 2024 18:05
@phsft-bot
Copy link

Can one of the admins verify this patch?

@hahnjo
Copy link
Member

hahnjo commented Feb 13, 2024

@phsft-bot build

@phsft-bot
Copy link

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

@dpiparo
Copy link
Member

dpiparo commented Feb 14, 2024

Hello: for me this PR is good to go @lmoneta ?

vepadulano
vepadulano approved these changes Feb 14, 2024
View reviewed changes
Copy link
Member

@vepadulano vepadulano left a comment

Choose a reason for hiding this comment

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

LGTM, @lmoneta have a look when you can

@lmoneta
Copy link
Member

lmoneta commented Feb 14, 2024

The PR looks good. The doubt I have is that there is a change in functionality. In ROOT, all random generators of TRandom::Random() return a value in (0,1], excluding the zero. Should we add a maybe +1 to the generated integer numbers in [0,int_max], before converting to float ?

@kjvbrt kjvbrt requested a review from bellenot as a code owner February 14, 2024 11:37
@hahnjo
Copy link
Member

hahnjo commented Feb 14, 2024

@phsft-bot build

@phsft-bot
Copy link

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

@phsft-bot
Copy link

Build failed on ROOT-ubuntu2004/python3.
See console output.

@kjvbrt
Copy link
Contributor Author

kjvbrt commented Feb 14, 2024

The PR looks good. The doubt I have is that there is a change in functionality. In ROOT, all random generators of TRandom::Random() return a value in (0,1], excluding the zero. Should we add a maybe +1 to the generated integer numbers in [0,int_max], before converting to float ?

Adding +1 to the generated int which is equal to int_max will result again in 0 (zero), no?

lmoneta
lmoneta reviewed Feb 14, 2024
View reviewed changes
math/mathcore/src/TRandom3.cxx Outdated
if (y) return ( (Double_t) y * 2.3283064365386963e-10); // * Power(2,-32)
return Rndm();
// 2.3283064365386963e-10 == 1. / (max<UInt_t> + 1) -> then returned value cannot be = 1.0
return static_cast<Double_t>(y * 2.3283064365386963e-10);
Copy link
Member

Choose a reason for hiding this comment

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

We can add the +1 here after converting to double to avoid overflows in the uint:

return (static_cast<Double_t>(y) + 1)* 2.3283064365386963e-10)

Copy link
Member

Choose a reason for hiding this comment

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

The overhead of adding a +1 should be minimal. We could test running tutorial/math/testRandom.C

Copy link
Member

Choose a reason for hiding this comment

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

@kjvbrt could you take care?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'm testing the implementation with +1 ATM.

Copy link
Contributor Author

@kjvbrt kjvbrt Feb 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is the result:

                   TRandom3    TRandom3 with +1       
Rndm..............   20.000      20.000                                               
RndmArray.........   10.000      15.000                                               
Gaus..............   60.000      60.000                                               
Rannor............   40.000      40.000                                               
Landau............   40.000      40.000                                               
Exponential.......   35.000      35.000
Binomial(5,0.5)...  125.000     130.000
Binomial(15,0.5)..  370.000     380.000
Poisson(3)........   90.000      95.000
Poisson(10).......  205.000     210.000
Poisson(70).......  295.000     300.000
Poisson(100)......  295.000     295.000
GausTF1...........  180.000     180.000
LandauTF1.........  175.000     175.000

@hahnjo
Copy link
Member

hahnjo commented Feb 14, 2024

@phsft-bot build

@phsft-bot
Copy link

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

@phsft-bot
Copy link

Build failed on ROOT-ubuntu2004/python3.
Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link

Build failed on ROOT-ubuntu2204/nortcxxmod.
Running on root-ubuntu-2204-2.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link

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

Failing tests:

And 1 more

@phsft-bot
Copy link

Build failed on ROOT-performance-centos8-multicore/soversion.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Failing tests:

And 3 more

@kjvbrt kjvbrt mentioned this pull request Feb 15, 2024
Open
@phsft-bot
Copy link

Build failed on mac12arm/cxx20.
Running on 194.12.161.128:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

And 5 more

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 15, 2024 •
edited
Loading

Test Results

    12 files      12 suites   2d 3h 59m 38s ⏱️
 2 561 tests  2 542 ✅ 0 💤  19 ❌
28 812 runs  28 659 ✅ 0 💤 153 ❌

For more details on these failures, see this check.

Results for commit f501b84.

♻️ This comment has been updated with latest results.

vepadulano
vepadulano requested changes Feb 16, 2024
View reviewed changes
Copy link
Member

@vepadulano vepadulano left a comment

Choose a reason for hiding this comment

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

The changes of this PR seem to trigger many errors in our CI. I will remove my approval pending a fix for the tests

@dpiparo
Copy link
Member

dpiparo commented Feb 28, 2024

@lmoneta @kjvbrt it seems to me we are very near to the fix: what is missing to make these tests succeed?

@dpiparo dpiparo closed this Nov 30, 2024
@dpiparo dpiparo reopened this Nov 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lmoneta lmoneta lmoneta left review comments

@dpiparo dpiparo dpiparo left review comments

@vepadulano vepadulano vepadulano requested changes

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

Requested changes must be addressed 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.

[ROOT-9733] TRandom3 does not implement perfectly a Mersenne Twister PRNG
7 participants
@kjvbrt @phsft-bot @hahnjo @dpiparo @lmoneta @vepadulano @ferdymercury