[math] Make TRandom compatible with UniformRandomBitGenerator - Issue # 7979#22508
[math] Make TRandom compatible with UniformRandomBitGenerator - Issue # 7979#22508medeirosdev wants to merge 3 commits into
Conversation
TRandom and its subclasses can now be used directly with standard library algorithms like std::shuffle and std::uniform_int_distribution. This is done by adding the three required members to TRandom: result_type, min(), max(), and operator(). The operator() delegates to the virtual Rndm(), so polymorphism is preserved across all TRandom subclasses. Fixes root-project#7979
| // NOTE: Rndm() returns a double in ]0,1[, so converting back to UInt_t | ||
| // has a small precision loss. Subclasses with access to raw integer output | ||
| // could override this for better accuracy. | ||
| result_type operator()() { return static_cast<result_type>(Rndm() * (static_cast<double>(max()) + 1.0)); } |
There was a problem hiding this comment.
| result_type operator()() { return static_cast<result_type>(Rndm() * (static_cast<double>(max()) + 1.0)); } | |
| virtual result_type operator()() { return static_cast<result_type>(Rndm() * (static_cast<double>(max()) + 1.0)); } |
since you mention that we allow overriding
I'd also suggest doing the overriding also here in this PR in TRandom1/2/3 classes to avoid division and then remultiplication by fScale
| using result_type = UInt_t; | ||
| static constexpr result_type min() { return 0; } | ||
| static constexpr result_type max() { return std::numeric_limits<UInt_t>::max(); } | ||
| // NOTE: Rndm() returns a double in ]0,1[, so converting back to UInt_t |
There was a problem hiding this comment.
thanks for this!
Please convert to doxygen format
|
also to discuss whether this can be unified with tmva/tmva/inc/TMVA/Tools.h to avoid code duplicities |
- Convert // NOTE: comment to Doxygen /// \note format in TRandom.h - Make TRandom::operator()() virtual so subclasses can override it - Add TRandom2::operator()() returning raw Tausworthe XOR output, avoiding the double round-trip through Rndm() / kScale - Add TRandom3::operator()() returning raw Mersenne Twister output, same motivation as above - Extend testMathRandom test5() to exercise TRandom2 as a URBG TRandom1 (RANLUX) has a float-based internal state with no meaningful raw integer path, so it keeps the base-class fallback. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Thanks for the review! Addressed all three points:
For TRandom1 (RANLUX), the internal state is float-based (fFloatSeedTable), so there's no raw integer to expose — the base class fallback is the best we can do there. |
The main difference is that our approach embeds the interface directly in TRandom, so any existing TRandom* pointer works as a URBG without wrapping. It also lets subclasses override operator()() to return raw integers directly (as done here for TRandom2 and TRandom3), which TMVA::RandomGenerator doesn't do, it goes through Integer() which still uses a double internally. i'm open to explore that in a follow-up PR if that's the preferred direction. |
There was a problem hiding this comment.
Could we call here the overridden operator () to avoid duplicities and save a few lines code? Same for Trandom3
Avoids duplicating the Tausworthe/Mersenne Twister state-advance logic. Rndm() now calls operator()() to get the raw integer and then scales it to a double, keeping a single source of truth for the generator core. The TAUSWORTHE macro is moved to file scope so RndmArray can still use it.
| const double kScale = 2.3283064365386963e-10; // range in 32 bit ( 1/(2**32) | ||
|
|
There was a problem hiding this comment.
kScale could become constexpr
|
@guitargeek @hageboeck hey guys! could you check this pr? please |
TRandom and its subclasses can now be used directly with standard library algorithms like std::shuffle and std::uniform_int_distribution.
This is done by adding the three required members to TRandom: result_type, min(), max(), and operator(). The operator() delegates to the virtual Rndm(), so polymorphism is preserved across all TRandom subclasses.
Fixes #7979
This Pull request:
Changes or fixes:
Checklist:
This PR fixes #