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
Semi-random initial value in the L1 trigger prescale counter #37506
Semi-random initial value in the L1 trigger prescale counter #37506
Conversation
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37506/29216
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
d38aed1
to
05f0658
Compare
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37506/29218
|
A new Pull Request was created by @silviodonato (Silvio Donato) for master. It involves the following packages:
@epalencia, @cmsbuild, @cecilecaillol, @rekovic can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a couple of comments/questions for my own understanding (the HLT signature obviously isn't required in this PR).
@@ -164,6 +165,32 @@ namespace l1t { | |||
/// pointer to Tau data list | |||
inline const BXVector<const GlobalExtBlk*>* getCandL1External() const { return m_candL1External; } | |||
|
|||
//initializer prescale counter using a semi-random value between [1, prescale value] | |||
static const std::vector<double> semirandomNumber(const edm::Event& iEvent, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why static
, rather than just a const
class member?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to stress that this is a standalone function, that might be used even outside a GlobalBoard object.
I might define it as an external function eg. l1t::semirandomNumber(...)
, but I don't know in which file I should define this function. I've just learned that I cannot define it as l1t::GlobalBoard::semirandomNumber(...)
because a namespace cannot have the same name of a class.
I don't have a strong opinion. What do you suggest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No strong opinion here, either. :)
I guess l1t::GlobalBoard::semirandomNumber(..)
is effectively how this function would be called outside this class if using the current implementation. I don't see a good place for this elsewhere in this package, so unless L1T has a better idea, I would leave it here like this (maybe the implementation could be moved to the .cc
file, though).
} | ||
return out; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this never happens for the values used by this function, but for double
s in general I think the assert
s above could fail (given floating-point precision and the fact that out[i]
is the result of a computation). Is that why they are there?
The current impl only handles the case of non-fractional prescales, right?
Should the randomisation be per algo (meaning, a different rand
for each algo), or not (like in the current impl)?
In case it helps, I find the impl below a bit more readable (but I didn't test it, and there are likely better ways than using std::round
to check for integers).
auto out = prescaleFactorsAlgoTrig;
// pick a random number from a combination of run, lumi, event numbers (different number for different threads)
std::srand(iEvent.id().run());
std::srand(std::rand() + iEvent.id().luminosityBlock());
// this causes different (semi)random number number for different streams
// reminder: different streams have different initial event number
std::srand(std::rand() + iEvent.id().event());
// (semi)random number in [0,1] range
auto const semirandom = std::rand() / double(RAND_MAX);
for (auto& ps : out) {
// if the ps is smaller than 1 (e.g. ps=0, ps=1) or if the prescale is fractional, it is not changed
// else, replace ps with a semirandom integer in the [1,ps] range
if (ps > 1 and ps == std::round(ps)) {
ps = std::round((ps - 1)*semirandom + 1);
}
}
return out;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like my reply is lost.
-
I replaced the assert with a warning. In case of problems it will simply use the standard method ( take PS )
-
I think that it should work also with fractional prescales, as they don't have a specific treatment in GlobalBoard.cc (not 100% sure though)
-
rand
should be common to among all the algos in order to preserve the synchronization of the L1 prescales (eg. L1_ZeroBias1 with PS = 20 should be always in the shadow of L1_ZeroBias2 with PS =10 ) -
thanks, I partially copied your code. I had to make some change to preserve the behavior described above.
m_prescaleCounterAlgoTrig.push_back(semirandomNumber(iEvent, prescaleFactorsAlgoTrig)); | ||
} else { | ||
m_prescaleCounterAlgoTrig.push_back(prescaleFactorsAlgoTrig); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it could be done outside of the loop on totalBxInEvent
(wouldn't we be recalculating the same semirandom PSs in each round of that loop, since run-lumi-event do not change?).
By the way, the m_prescaleCounterAlgoTrig.reserve
call (above the diff) seems wrong. I was expecting m_prescaleCounterAlgoTrig.reserve(totalBxInEvent)
.
[..]
m_prescaleCounterAlgoTrig.reserve(totalBxInEvent);
auto const& prescaleCountersAlgoTrig = m_semiRandomInitialPSCounters ? semirandomNumber(iEvent, prescaleFactorsAlgoTrig) : prescaleFactorsAlgoTrig;
for (int iBxInEvent = 0; iBxInEvent <= totalBxInEvent; ++iBxInEvent) {
m_prescaleCounterAlgoTrig.push_back(prescaleCountersAlgoTrig);
[..]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks I agree with both comments
05f0658
to
17833d5
Compare
@@ -971,10 +971,13 @@ void l1t::GlobalBoard::runFDL(edm::Event& iEvent, | |||
// prescale counters are reset at the beginning of the luminosity segment | |||
if (m_firstEv) { | |||
// prescale counters: numberPhysTriggers counters per bunch cross | |||
m_prescaleCounterAlgoTrig.reserve(numberPhysTriggers * totalBxInEvent); | |||
m_prescaleCounterAlgoTrig.reserve(totalBxInEvent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cms-sw/l1-l2 please note this is the only line which should affect the default behavior (semiRandomInitialPSCounters = false)
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37506/29637
|
Pull request #37506 was updated. @epalencia, @cmsbuild, @cecilecaillol, @rekovic can you please check and sign again. |
if (ps > 1) { | ||
auto nps = semirandom - floor(semirandom / ps) * ps; | ||
// if nps=0 or a wrong value (<0,>ps) use PS value (standard method) | ||
if (nps > 0 || nps <= ps) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (nps > 0 || nps <= ps) | |
if (nps > 0 and nps <= ps) |
No?
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37506/30038
|
Pull request #37506 was updated. @epalencia, @cmsbuild, @cecilecaillol, @rekovic can you please check and sign again. |
please test I suggest to start testing. |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-265261/24762/summary.html Comparison Summary@slava77 comparisons for the following workflows were not done due to missing matrix map:
Summary:
|
@cms-sw/l1-l2 do you have any comments? |
+l1 |
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-265261/24965/summary.html Comparison SummarySummary:
|
+1 |
PR description:
The standard behavior of the L1 trigger prescaler is to start a counter from the prescale value (eg. 1000) and then reduce it of one unit every time the corresponding L1 trigger is accepted. Once the counter reach 0, the L1 trigger is consider to pass also the L1 prescale, and the counter goes back to the prescale value.
The counter is reset at the beginning of each lumisection (unless you enable #37395)
This PR adds the option to set the initial prescale value of the PS counter from PS to a semirandom value taken in the range [0, PS]. This option is required to avoid that running on a heavily prescaled sample (eg. ZeroBias) the PS counter never reach 0 and therefore no events pass the prescaler. Starting on a random value, the average rate will be closer to the actual rate that we would see in the actual datataking (at P5 the L1 trigger evaluate a huge number of events per LS).
The "semi-random" refers to the
std::srand
function which gives a reproducible sequence of random numbers.However the reproducibility is not preserved when running with multithreading because I use also the event number as input of the random number generator.
The current L1 trigger precaler does not preserve the reproducibility when multi-threading is on.
This PR is somehow related to #37395.
PR validation:
Running the prescaler on 3151 events (two LS: 1584 events + 1567 events) on ZeroBias with a prescale of 100 using 16 threads.
Default configuration: 8 events passed
Including this PR (default): 6 events passed
Including this PR (semiRandomInitialPSCounters on): 30 events passed
Second test (same configurations)
Default configuration: 4 events passed
Including this PR (default): 5 events passed
Including this PR (semiRandomInitialPSCounters on): 31 events passed
if this PR is a backport please specify the original PR and why you need to backport that PR:
Yes, I'd like to have a backport to 12_3_X - although it is not strictly necessary - as it will be used for the first stable beam and therefore for the first HLT menu.
We use this PR for the HLT rate estimate based on the L1 skim of the ZeroBias dataset.