Skip to content
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.

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

feat: settable seed per CalorimeterHitDigi algorithm #800

Closed
wants to merge 1 commit into from

Conversation

wdconinc
Copy link
Contributor

Briefly, what does this PR introduce?

This adds a settable seed (uint32_t) for the CalorimeterHitDigi algorithm, defaulting to zero, but settable in the derived factories. If we move to a different structure of factories, they should be able to produce the exact same output, per algorithm/factory, if given the same seed. Not sure this really gives us that much more insight or control, but it gives us a way to make sure we can control each algorithm independently.

What kind of change does this PR introduce?

  • Bug fix (issue #__)
  • New feature (issue #__)
  • Documentation update
  • Other: __

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

No.

Does this PR change default behavior?

No.

This adds a settable seed (uint32_t) for the CalorimeterHitDigi
algorithm, defaulting to zero, but settable in the derived factories. If
we move to a different structure of factories, they should be able to
produce the exact same output, per algorithm/factory, if given the same
seed. Not sure this really gives us that much more insight or control,
but it gives us a way to make sure we can control each algorithm
independently.
@veprbl
Copy link
Member

veprbl commented Jul 26, 2023

I am confused: according to https://cplusplus.com/reference/random/linear_congruential_engine/linear_congruential_engine/, the default_seed is already set to 1u. What is the issue with using that value?

@wdconinc
Copy link
Contributor Author

What is the issue with using that value?

Nothing wrong with that value, other than it's the same for all factories.

I'm worried about #794, which shouldn't have changed anything in the digitization randomness if the random_number_engine was doing the right thing. I'm worried about there's something about the way the factories are (now) created with algorithm as a member that makes the default random engine initialization different compared to the previous model of a factory inheriting from the algorithm, e.g. the factory is created in the factory generator, but then copied or something else that doesn't deal well with the random engine state (I don't think a copy of a random engine necessarily gives you a second random engines that stays in sync).

So, I want to make sure we explicitly seed the random engine on init with a value that allows us to make sure that that it was that configuration that caused the seed, not just the default random seed (1u and 0u both result in the same internal state, which would be nice if https://en.cppreference.com/w/cpp/numeric/random/linear_congruential_engine/seed mentioned that).

@wdconinc
Copy link
Contributor Author

But I don't like adding more stuff the config, certainly not random stuff. Ideally I can figure out what's going on with #794 without needing to merge this.

@wdconinc wdconinc closed this Jul 27, 2023
@wdconinc wdconinc deleted the digi-with-seed branch July 27, 2023 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants