[math] PRBS generator

[ferdymercury]

This Pull request:

Changes or fixes:

Adds PRBS generation code

Checklist:

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

This PR fixes #8199

[couet]

LGTM

[hahnjo]

@phsft-bot build

I think new generators should go via ROOT::Math::TRandomEngine, not another TRandomXYZ, right @lmoneta? Also would be good to have some tests.

[dpiparo]

I would like to revive this PR, @ferdymercury do you feel like rebasing so that I can start the builds ?

[hahnjo]

see also #8798 (comment)

[github-actions]

Test Results

    20 files      20 suites   3d 14h 2m 12s ⏱️
 3 792 tests  3 792 ✅ 0 💤 0 ❌
74 094 runs  74 094 ✅ 0 💤 0 ❌

Results for commit 1a44726.

♻️ This comment has been updated with latest results.

[ferdymercury]

@hahnjo Thanks for the comment! I can move it there, but do I really need to derive from TRandomEngine? The parent methods are for double Rndm(), which does not seem very useful to me.
This generator is a binary register generator, so rather a quite different structure, and it is not intended to be used as a normal generator, but rather as a test scenario or helper math functions for electronics testing. It also is inherently templated, etc. It returns an array rather than just a number Rndm(). See https://github.com/root-project/root/pull/8798/files#diff-2e848ceefaff2e24c9b2970fb86a8da1d3d00603fc4f48f920421e603198fab2

Wrt tests, I will 'copy' the mentioned tutorial as 'test' once it's clear where this class should go.

Thanks for the review!! :)

[lmoneta]

Hello @ferdymercury
I am not sure what the use case is for having this generator in ROOT. One could, in principle, also use the bits of the generated numbers of a good generator like RANLUX++.
Do we have users that are asking for this feature?

[ferdymercury]

Hello @ferdymercury I am not sure what the use case is for having this generator in ROOT. One could, in principle, also use the bits of the generated numbers of a good generator like RANLUX++. Do we have users that are asking for this feature?

Thanks for the reply! We discussed this point in the associated issue: #8199 (comment)

[lmoneta]

I still think is very specific, and we could have it maybe as an external addition.

[ferdymercury]

Ok. What do you mean by external? Moving it to just a tutorial? Happy to change that once it's clarified.

[silverweed]

@lmoneta any chance of moving this forward? Not necessarily by merging it, but to know how to proceed (maybe move the implementation to a tutorial as @ferdymercury suggested)

[guitargeek]

Hi @ferdymercury, all, I think I know how to move this forward. Indeed, using the TRandom* name is not optimal, as the mental model that we have then is pseudorandom number generation for floating point numbers, which confuses users and maintainers.

Indeed, we nowadays prefer to have things in the ROOT::Math namespace, so maybe your class can live there as free functions in a ROOT::Math::PRBS or ROOT::Math::LFSR namespace? As far as I see, your proposed class has only static methods, so it might as well be as set of two free functions. The definition can then go in a new mathcore/inc/Math/PRBS.h (or LFSR.h, I'd be fine with both).

Would that be okay for you? If this change would be made, and some tests implemented, I'd have no problem merging this! I see no risk. The maintenance burden is minimal, as there would be no new serializable class, no new dependencies, and minimal code. And there is also no risk of confusion if this is clearly separate from TRandom*. The usecase is also clearly described in #8199 (comment), and implementing this would be a nice gesture to our users who use ROOT in more hardware-near lab environments.

@ferdymercury, what do you think? Sorry that this PR took so long.

[ferdymercury]

things in the ROOT::Math namespace, so maybe your class can live there as free functions in a ROOT::Math::PRBS or ROOT::Math::LFSR namespace? As far as I see, your proposed class has only static methods, so it might as well be as set of two free functions. The definition can then go in a new mathcore/inc/Math/PRBS.h (or LFSR.h, I'd be fine with both).

Would that be okay for you?

Sounds perfect! I can take a look on it, thanks for the feedback.

[guitargeek]

Some suggestions/requests for improvement inline.

[guitargeek]

Thanks for the update @ferdymercury! So in the end you didn't want to go for the iterators? I have made a suggestion to address your problem stated in #8798 (comment). Not sure if you have seen it, because GitHub is glitching for me (I can't see my own comments from my phone for example....).

What comes to my mind is to introduce an additional bool buffer variable, and do something like this instead:

if (i > 0)
   *out++ = buffer;
buffer = newBit;

And then in the end:

if(!oppositBit)
    *out++ = buffer; // don't add the last element otherwise, as we already pushed the one from the seed above the while loop

So there are ways, and it's generally cleaner to avoid these vector specific things like pop_back() and shrink_to_fit() IMHO. Also, with this pattern you might the unnecessary re-allocations.

Or you really want a return variable?

[guitargeek]

I see we don't agree on some of the subjective choices here 😄
No problem. Then my final questions are if the taps is worth to pass by const &, and why we actually have the static keywords.

[guitargeek]

Ooooh okay I get it now, GitHub only posts all my inline comments after I finish the review! 😮 It's the first time I use the right button for inline review comments, and I got confused. So that's what "pending" meant.... I was already surprised how you ignored so professionally my comments 🤣 Well, anyway, now you should be able to see all my suggestions.

[ferdymercury]

Not sure if you have seen it, because GitHub is glitching for me (I can't see my own comments from my phone for example....).

Ah, nope, sorry, I didn't see that, might have been a glitch.

Or you really want a return variable?

There are several reasons I preferred a return variable:

• Users saves three lines of code:
  std::vector<whatever> mycontainer;
auto maxPeriod = pow(2,k-1);
mycontainer.reserve(maxPeriod);
  Especially the second one, if the user forgets it or has it wrong, then we might get problems when working with iterators.
• Size is not totally predictable in advance, it depends on the initial seed, again problems if you have iterators. The algorithm stops when a register value is repeated (or maxPeriod is reached)
• I am less experienced with iterators, so maybe if these things can be solved, I might not know how to solve these easily. That said, with help on how to resolve these issues, I could consider changing to them :)