deprecate integer types for L1T and HLT prescales in HLTConfigData and its clients
#39220
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39220/31842
|
|
A new Pull Request was created by @missirol (Marino Missiroli) for master. It involves the following packages:
@malbouis, @tvami, @yuanchao, @pmandrik, @rekovic, @epalencia, @mandrenguyen, @emanueleusai, @ahmad3213, @saumyaphor4252, @cmsbuild, @missirol, @ChrisMisan, @jfernan2, @clacaputo, @Martin-Grunewald, @jpata, @francescobrivio, @micsucmed, @cecilecaillol, @rvenditti can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8aa7bf/27131/summary.html Comparison SummarySummary:
|
|
test parameters:
|
|
please test workflow 11634.0 |
|
please abort Wf 11634.0 runs by default in PR tests, and already passed in the first round of tests (see above). |
|
test parameters: |
There was a problem hiding this comment.
@missirol I think it should be considered to implement a separate object for constructions like pair of vector of pairs of... It'd make the code far more clear and it'd also tackle the issue of abused auto statement.
| @@ -199,7 +199,7 @@ class IsoTrig : public edm::one::EDAnalyzer<edm::one::WatchRuns, edm::one::Share | |||
| math::XYZPoint leadPV_; | |||
|
|
|||
| std::map<unsigned int, unsigned int> trigList_; | |||
| std::map<unsigned int, const std::pair<int, int>> trigPreList_; | |||
| std::map<unsigned int, const std::pair<double, int>> trigPreList_; | |||
There was a problem hiding this comment.
This can be unordered_map
| std::map<unsigned int, unsigned int>::iterator itr; | ||
| std::map<unsigned int, const std::pair<int, int>>::iterator itrPre; | ||
| std::map<unsigned int, const std::pair<double, int>>::iterator itrPre; |
There was a problem hiding this comment.
This can be unordered_map
| @@ -788,13 +788,12 @@ void PATTriggerProducer::produce(Event& iEvent, const EventSetup& iSetup) { | |||
| const edm::TriggerNames& names = iEvent.triggerNames(*handleTriggerResults); | |||
| //std::cout << "Run " << iEvent.id().run() << ", LS " << iEvent.id().luminosityBlock() << ": pset " << set << std::endl; | |||
| auto l1HLTPSFrac = hltPSProvider_.prescaleValues<FractionalPrescale>(iEvent, iSetup, hltPath_); | ||
| auto l1HLTPSDoubleFrac = hltPSProvider_.prescaleValues<double, FractionalPrescale>(iEvent, iSetup, hltPath_); | ||
|
|
||
| auto l1HLTDetailPSDouble = hltPSProvider_.prescaleValuesInDetail<double>(iEvent, iSetup, hltPath_); | ||
| auto l1HLTDetailPSInt = hltPSProvider_.prescaleValuesInDetail<int>(iEvent, iSetup, hltPath_); | ||
| auto l1HLTDetailPSInt = hltPSProvider_.prescaleValuesInDetail<double, int>(iEvent, iSetup, hltPath_); | ||
| auto l1HLTDetailPSFrac = hltPSProvider_.prescaleValuesInDetail<FractionalPrescale>(iEvent, iSetup, hltPath_); | ||
|
|
||
| std::cout << "---------Begin Event--------" << std::endl; |
There was a problem hiding this comment.
Please consider changing couts to logging system.
There was a problem hiding this comment.
so I disagree with this comment and I will explain my reasoning.
The HLTPrescaleExample module is clearly a test/example module which will never ever run in normal CMSSW jobs as it doesnt do anything.
Its purpose is to help users hence the simplicity.
Even though I have 15 years on CMSSW, I still find configuring the message logger properly a bit of trial and error. Perhaps this is just me but perhaps not. But I figure my target audience, a new grad student has no hope.
If this was to run other than test/example, I would fully agree with changing it but given its purpose is a simple example to try and quickly help users who dont want to waste time dealing with the message logger, I would have left it as is.
There was a problem hiding this comment.
I can revert this change. Regarding the cfg file, what would you prefer? (keep its update or not, use it or not as a unit test)
There was a problem hiding this comment.
ah yes you are now using it as a unit test which means it probably should be message logger. Anyways thought I would explain why it was done with couts in the first place.
The issue that it would be very nice that this example prints out some output so we would need to configure the example properly configure the message logger.
I envisioned the user running it as a test and then having the prescales dump to screen which I think is usually useful for learning and getting a feel if its working.
There was a problem hiding this comment.
I see. Indeed, I didn't touch it initially, then I updated it such that cmsRun hltPrescaleExample_cfg.py (which is all the unit test is) gives the full printout, a la cout. I leave it as is, unless you let me know otherwise.
|
please test Refreshing the tests before signing for HLT. @cms-sw/xpog-l2 , do you have any comments on this PR? |
|
integration test under consideration https://gitlab.cern.ch/cms-nanoAOD/nanoAOD-integration/-/issues/190 |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8aa7bf/27642/summary.html Comparison SummarySummary:
|
|
+hlt |
|
+1 |
|
Hi @missirol |
|
urgent
|
|
@cms-sw/orp-l2 I understand |
A last-minute backport of a big PR is exactly what I was trying to avoid, thus the comments in #39220 (comment) and #39220 (comment), specifically
Later today, I will have to check if a backport is trivial, or requires more work, assuming ORP is okay with it. |
missirol
changed the title
HLTConfigProvider and its clientsHLTConfigData and its clients
|
type bugfix A bi-product of this PR is a bugfix to MINIAOD in the handling of L1T prescales. Based on #39220 (comment), a backport to |
| @@ -235,11 +235,8 @@ void EwkMuLumiMonitorDQM::analyze(const Event& ev, const EventSetup&) { | |||
| bool prescaled = false; | |||
| const unsigned int prescaleSize = hltConfigProvider_.prescaleSize(); | |||
| for (unsigned int ps = 0; ps < prescaleSize; ps++) { | |||
| const unsigned int prescaleValue = hltConfigProvider_.prescaleValue(ps, trig); | |||
| if (prescaleValue != 1) | |||
| if (hltConfigProvider_.prescaleValue<double>(ps, trig) != 1) | |||
There was a problem hiding this comment.
Aren't you affected by possible rounding effects of the double quantity here?
There was a problem hiding this comment.
See #39220 (comment).
| const std::pair<int, int> prescales(hltPrescaleProvider_.prescaleValues(iEvent, iSetup, *iHLT)); | ||
| if (prescales.first * prescales.second == 1) | ||
| auto const prescales = hltPrescaleProvider_.prescaleValues<double>(iEvent, iSetup, *iHLT); | ||
| if (prescales.first == 1 and prescales.second == 1) |
There was a problem hiding this comment.
Is this ok with rounding effects?
There was a problem hiding this comment.
See #39220 (comment). Here, I changed the check to "HLT==1 and L1T==1", instead of doing a floating-point operation ("HLT * L1T").
| @@ -139,7 +139,7 @@ void TriggerMatchProducer<object>::produce(edm::Event& event, const edm::EventSe | |||
|
|
|||
| if (TString(*iHLT).Contains(TRegexp(hltTag_))) { | |||
| triggerInMenu[*iHLT] = true; | |||
| if (hltPrescaleProvider_.prescaleValue(event, eventSetup, *iHLT) == 1) | |||
| if (hltPrescaleProvider_.prescaleValue<double>(event, eventSetup, *iHLT) == 1) | |||
There was a problem hiding this comment.
Is this ok with rounding effects?
There was a problem hiding this comment.
To the best of my knowledge, yes, in short because that double is not the result of floating-point operation.
This call goes back to HLTConfigData::prescaleValue<T>, where we have the implicit conversion of HLTPrescaleTable::prescale (unsigned int) into T (which must be double or FractionalPrescale). So, we are reading an int as a double, not calculating this double with a floating-point operation (in that case, the complication of rounding effects would apply, afaiu).
The unit test in HLTrigger/HLTcore/test/test_catch2_HLTConfigData.cc was extended exactly to gain some confidence in this respect, comparing double, or FractionalPrescale, to unsigned int for the values of a dummy PS table.
| ++iSeed) { | ||
| int l1 = (hltPrescale_->prescaleValuesInDetail(iEvent, iSetup, hltpath1)).first.at(iSeed).second; | ||
| for (size_t iSeed = 0; iSeed < prescales_num.first.size(); ++iSeed) { | ||
| auto l1 = prescales_num.first.at(iSeed).second; | ||
| if (l1 < 1) | ||
| continue; | ||
| if (l1 == 1) { |
There was a problem hiding this comment.
Is this ok with rounding effects?
There was a problem hiding this comment.
See #39220 (comment).
| @@ -108,7 +108,7 @@ void TriggerCandProducer<object>::produce(edm::Event& event, const edm::EventSet | |||
| for (std::vector<edm::InputTag>::const_iterator iMyHLT = hltTags_.begin(); iMyHLT != hltTags_.end(); ++iMyHLT) { | |||
| if ((*iMyHLT).label() == *iHLT) { | |||
| triggerInMenu[(*iMyHLT).label()] = true; | |||
| if (hltPrescaleProvider_.prescaleValue(event, eventSetup, *iHLT) == 1) | |||
| if (hltPrescaleProvider_.prescaleValue<double>(event, eventSetup, *iHLT) == 1) | |||
There was a problem hiding this comment.
Is this ok with rounding effects?
There was a problem hiding this comment.
See #39220 (comment).
| @@ -411,7 +411,7 @@ void EopElecTreeWriter::analyze(const edm::Event& iEvent, const edm::EventSetup& | |||
|
|
|||
| const unsigned int prescaleSize = hltConfig_.prescaleSize(); | |||
| for (unsigned int ps = 0; ps < prescaleSize; ps++) { | |||
| const unsigned int prescaleValue = hltConfig_.prescaleValue(ps, triggerName); | |||
| auto const prescaleValue = hltConfig_.prescaleValue<double>(ps, triggerName); | |||
| if (prescaleValue != 1) { | |||
There was a problem hiding this comment.
Is this ok with rounding effects?
There was a problem hiding this comment.
See #39220 (comment).
| @@ -45,33 +42,14 @@ namespace heppy { | |||
| } | |||
| bool outcome = true; | |||
| for (std::vector<unsigned int>::const_iterator it = indices_.begin(), ed = indices_.end(); it != ed; ++it) { | |||
| if (result.getPrescaleForIndex(*it) != 1) { | |||
| if (result.getPrescaleForIndex<double>(*it) != 1) { | |||
There was a problem hiding this comment.
Is this ok with rounding effects?
There was a problem hiding this comment.
See #39220 (comment).
| @@ -558,7 +558,7 @@ void PATTriggerProducer::produce(Event& iEvent, const EventSetup& iSetup) { | |||
| if (hltConfig.prescaleSize() > 0) { | |||
| if (hltPrescaleProvider_.prescaleSet(iEvent, iSetup) != -1) { | |||
There was a problem hiding this comment.
Is this ok with rounding effects?
There was a problem hiding this comment.
prescaleSet is the index of the PS column, and it remains an int, so no issue.
|
+1 |
|
merge |
PR description:
#32741 introduced the use fractional L1T prescales [1], as well as support for them in HLT utilities [2].
[1] Here, "fractional" means a number corresponding to
N / 100, where N is non-negative integer.[2] This mainly concerns
HLTConfigProviderandHLTPrescaleProvider, which are used by many modules downstream of HLT to access the values of both L1T and HLT prescales.On the other hand, the default template arguments of some methods in [2] still lead to using integers for both L1T and HLT prescales. This can lead to silent truncation of non-integer L1T prescales in client code (in principle, this would also apply to HLT prescales, but non-integer HLT prescales have never been used, to my knowledge; conversely, fractional L1T prescales have been used to take data since a while now).
This PR (1) deprecates the use of types other than
doubleandFractionalPrescalefor accessing L1T and HLT prescales viaHLTConfigProviderandHLTPrescaleProvider, and (2) updates client code in CMSSW. Concerning user code outside CMSSW, (1) will prevent it from compiling if it tries to access L1T or HLT prescales with deprecated types (the solution is to specify the appropriate types as template arguments).Edit: this PR was originally dropping support for integers in HLT utilities only for L1T prescales, and this was later extended to HLT prescales as well, with the idea of 'breaking' user code only once for both, in view of the possibility that HLT might use non-integer prescales in the future. See for further details.
This PR is alternative to #39094.
Kindly asking @Sam-Harper and @fwyzard to review.
Note : this PR only takes care of code accessing L1T prescales via
HLTPrescaleProvider. Clients accessing L1T prescales in other ways [a,b,c] are not affected (for better or worse) by this PR.[a] code accessing L1T prescales directly via
L1GtUtils(Stage-1 L1T, pre-2016; only supportingintfor L1T prescale values)[b] code accessing L1T prescales directly via
L1TGlobalUtil(Stage-1 L1T, 2016-now; only supportingdoublefor L1T prescale values since #32741)[c] code accessing prescales viaPackedTriggerPrescales(e.g.int ps = (PackedTriggerPrescales).getPrescaleForNamewill continue to silently truncate values).PR validation:
scram build runtestsandrunTheMatrix.py -l 11634.0passed.If this PR is a backport, please specify the original PR and why you need to backport that PR. If this PR will be backported, please specify to which release cycle the backport is meant for:
Not sure about backports. In my understanding, client code touched by this PR is accessing L1T prescales in 2022 Data incorrectly, truncating those values to integers, so there might be reasons to backport this to
12_5_X, or even12_4_X. Aside from DQM (and code outside CMSSW), the main downstream effect might be on MINIAOD, because of the update inPATTriggerProducerandDataFormats/PatCandidates.