give OnlineLuminosityRecord info to HLT's LumiMonitor plugin
#39859
Conversation
| online_pu = scalit->pileup(); | ||
| } else if (onlineMetaDataDigisHandle.isValid()) { | ||
| online_lumi = onlineMetaDataDigisHandle->instLumi(); | ||
| online_pu = onlineMetaDataDigisHandle->avgPileUp(); | ||
| } |
There was a problem hiding this comment.
The logic of this choice was inspired by
and then I noticed it is basically the same as
| process.fastTimerServiceClient.doPlotsVsPixelLumi = False | ||
| process.fastTimerServiceClient.scalLumiME = cms.PSet( | ||
| process.fastTimerServiceClient.onlineLumiME = cms.PSet( | ||
| folder = cms.string('HLT/LumiMonitoring'), |
There was a problem hiding this comment.
This is the only place I could find where the parameter names appeared, and needed to be changed.
| trkTopoToken_(doPixelLumi_ ? esConsumes<TrackerTopology, TrackerTopologyRcd>() | ||
| : edm::ESGetToken<TrackerTopology, TrackerTopologyRcd>()), |
There was a problem hiding this comment.
This is meant to avoid consuming the record when the plugin does not make use of it.
The following would be slightly more compact
| trkTopoToken_(doPixelLumi_ ? esConsumes<TrackerTopology, TrackerTopologyRcd>() | |
| : edm::ESGetToken<TrackerTopology, TrackerTopologyRcd>()), | |
| trkTopoToken_(doPixelLumi_ ? esConsumes<TrackerTopology, TrackerTopologyRcd>() : trkTopoToken_), |
but I haven't seen this approach elsewhere, and I don't know if this is a good idea.
The even-more-compact version
| trkTopoToken_(doPixelLumi_ ? esConsumes<TrackerTopology, TrackerTopologyRcd>() | |
| : edm::ESGetToken<TrackerTopology, TrackerTopologyRcd>()), | |
| trkTopoToken_(doPixelLumi_ ? esConsumes() : trkTopoToken_), |
unfortunately didn't compile:
error: operands to '?:' have different types 'edm::EDConsumerBaseESAdaptor<edm::Transition::Event>' and 'const edm::ESGetToken<TrackerTopology, TrackerTopologyRcd>'
91 | trkTopoToken_(doPixelLumi_ ? esConsumes()
| ~~~~~~~~~~~~~^~~~~~~~~~~~~~
92 | : trkTopoToken_),
| ~~~~~~~~~~~~~~~
There was a problem hiding this comment.
As an aside: if there is feedback on this technical point, I'd be interested to know (doing x_{bool ? a : x_} in the initialiser list, and the fact that one of the options doesn't compile, which is probably expected, but I'm wondering if there is a way to make it work).
There was a problem hiding this comment.
This does not seem like a good idea, because x_ would end up uninitialised ?
Simple example using integers and strings:
#include <iostream>
#include <string>
struct Data {
public:
Data(int x, std::string s) : x_{x}, s_{std::move(s)}
{}
Data() : x_{x_}, s_{s_}
{}
int x() const {
return x_;
}
std::string const& s() const {
return s_;
}
private:
int x_;
std::string s_;
};
int main(void) {
Data value{42, "answer"};
std::cout << value.s() << " is " << value.x() << std::endl;
Data data;
std::cout << data.s() << " is " << data.x() << std::endl;
return 0;
}The compiler warns that x_ is initialised:
$ g++ -std=c++17 -O3 -flto -g -Wall -fPIC -MMD -march=native -mtune=native -c test.cc -o test.cc.o
test.cc: In member function ‘Data::Data()’:
test.cc:9:15: warning: ‘this_6(D)->x_’ is used uninitialized [-Wuninitialized]
9 | Data() : x_{x_}, s_{s_}
| ^~
Worse, there is no warning about s_ being uninitialised, but I get a crash:
$ ./test
answer is 42
terminate called after throwing an instance of 'std::bad_alloc'
what(): std::bad_alloc
Aborted (core dumped)
There was a problem hiding this comment.
Indeed.. thanks the example, it's clearer now!
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39859/32758
|
|
A new Pull Request was created by @missirol (Marino Missiroli) for master. It involves the following packages:
@Martin-Grunewald, @emanueleusai, @ahmad3213, @cmsbuild, @missirol, @jfernan2, @syuvivida, @pmandrik, @micsucmed, @rvenditti can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
DQM/HLTEvF/plugins/LumiMonitor.cc
Outdated
| histograms.lumiVsLS = me; | ||
|
|
||
| me = booker.bookProfile("puVsLS", | ||
| "scal PU vs LS", | ||
| "online PU vs LS", |
There was a problem hiding this comment.
could you also change PU to pileup and LS to lumisection in the labels ?
DQM/HLTEvF/plugins/LumiMonitor.cc
Outdated
| @@ -267,6 +262,7 @@ void LumiMonitor::fillDescriptions(edm::ConfigurationDescriptions& descriptions) | |||
| edm::ParameterSetDescription desc; | |||
| desc.add<edm::InputTag>("pixelClusters", edm::InputTag("hltSiPixelClusters")); | |||
| desc.add<edm::InputTag>("scalers", edm::InputTag("hltScalersRawToDigi")); | |||
| desc.add<edm::InputTag>("onlineMetaDataDigis", edm::InputTag("hltOnlineMetaDataDigis")); | |||
| desc.add<std::string>("FolderName", "HLT/LumiMonitoring"); | |||
There was a problem hiding this comment.
could you change this to folderName, for consistency with all the other parameters that are lowercase ?
There was a problem hiding this comment.
Sure. For the 12_4_X backport, we might want to avoid this imho, to spare FOG from template migrations just because of this.
There was a problem hiding this comment.
OK.
How are the changes to the client configuration propagated to the online DQM ?
There was a problem hiding this comment.
That's a good question. So far, I only found one place, #39859 (comment). I'll double-check, and hopefully DQM reviewers will know better than me.
| if (doPlotsVsScalLumi_) | ||
| fillPlotsVsLumi(booker, getter, current_path, "VsScalLumi", scalLumiMEPSet_); | ||
| if (doPlotsVsOnlineLumi_) | ||
| fillPlotsVsLumi(booker, getter, current_path, "vs_lumi", onlineLumiMEPSet_); |
There was a problem hiding this comment.
| fillPlotsVsLumi(booker, getter, current_path, "vs_lumi", onlineLumiMEPSet_); | |
| fillPlotsVsLumi(booker, getter, current_path, "_vs_lumi", onlineLumiMEPSet_); |
| if (doPlotsVsPixelLumi_) | ||
| fillPlotsVsLumi(booker, getter, current_path, "VsPixelLumi", pixelLumiMEPSet_); | ||
| fillPlotsVsLumi(booker, getter, current_path, "vs_pixelLumi", pixelLumiMEPSet_); |
There was a problem hiding this comment.
| fillPlotsVsLumi(booker, getter, current_path, "vs_pixelLumi", pixelLumiMEPSet_); | |
| fillPlotsVsLumi(booker, getter, current_path, "_vs_pixelLumi", pixelLumiMEPSet_); |
| if (doPlotsVsPU_) | ||
| fillPlotsVsLumi(booker, getter, current_path, "VsPU", puMEPSet_); | ||
| fillPlotsVsLumi(booker, getter, current_path, "vs_pileup", puMEPSet_); |
There was a problem hiding this comment.
| fillPlotsVsLumi(booker, getter, current_path, "vs_pileup", puMEPSet_); | |
| fillPlotsVsLumi(booker, getter, current_path, "_vs_pileup", puMEPSet_); |
| if (doPlotsVsScalLumi_) | ||
| fillPlotsVsLumi(booker, getter, subsubdir, "VsScalLumi", scalLumiMEPSet_); | ||
| if (doPlotsVsOnlineLumi_) | ||
| fillPlotsVsLumi(booker, getter, subsubdir, "vs_lumi", onlineLumiMEPSet_); |
There was a problem hiding this comment.
| fillPlotsVsLumi(booker, getter, subsubdir, "vs_lumi", onlineLumiMEPSet_); | |
| fillPlotsVsLumi(booker, getter, subsubdir, "_vs_lumi", onlineLumiMEPSet_); |
and so on for the others
There was a problem hiding this comment.
Mh.. but one underscore is added here (I also added a screenshot to the description). Or do we really want the double underscore?
There was a problem hiding this comment.
ah... no, we definitely do not want the double underscore :-)
missirol
force-pushed
the
devel_onlineMetaDataToLumiMon
branch
from
38f4785
to
4ce015f
Compare
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39859/32763
|
|
Pull request #39859 was updated. @Martin-Grunewald, @emanueleusai, @ahmad3213, @cmsbuild, @missirol, @jfernan2, @syuvivida, @pmandrik, @micsucmed, @rvenditti can you please check and sign again. |
|
Tried to address the comments received so far. Regarding #39859 (comment), I probably need feedback from @cms-sw/dqm-l2. If it's useful, before we consider merging this and backporting it, I could make a 124X version of this PR to be tested in the online DQM (we would not see the empty HLT plots getting filled, because that requires rerunning the HLT, but we would verify that things still work okay). |
|
👍🏻 |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39859/32765
|
|
please test |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39859/32855
|
|
Pull request #39859 was updated. @Martin-Grunewald, @emanueleusai, @ahmad3213, @cmsbuild, @missirol, @jfernan2, @syuvivida, @pmandrik, @micsucmed, @rvenditti can you please check and sign again. |
|
please test In Run-3 data wfs, harvesting outputs The bin-by-bin DQM differences also show two TProfiles in I tried to fix this in 375912f; in the MC case, the 2 profiles wrt lumi (which would always be empty) are not produced anymore. I don't have other changes planned for this PR. |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-62cf54/28647/summary.html Comparison SummarySummary:
|
|
+hlt
|
|
@cms-sw/dqm-l2 , any comments on this PR? It would be nice to have its backport merged in 12_4_X sooner rather than later. |
|
testing the 12_4 backport at p5 now, apologies for the delay |
|
+1 |
cmsbuild
added
dqm-approved
fully-signed
and removed
dqm-pending
pending-signatures
labels
|
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, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
resolves #39756
PR description:
This PR address the 2nd item in the description of #39756, plus a related comment in #39756 (comment).
The
LumiMonitorDQM plugin, used inside HLT, is updated to read the values of online instantaneous luminosity and mean PU from the so-called 'online-metadata' digis, when the corresponding info from SCAL is not available (i.e. from Run 3 onwards).To improve clarity, some of the plot labels, transient variables, and configuration parameters are renamed. I changed "scal lumi/PU" to a more generic "online lumi/PU", but of course feel free to suggest better names.
Minor technical improvements (e.g.
const-ness of class data members) were done in the process.375912f fixes a couple of pixel-related harvesting outputs which depend on the outputs of the
LumiMonitorplugin; please see #39859 (comment) for details.(In case this is backported to
12_4_X, it should not warrant a newConfDBparsing, since the new configuration parameter ofLumiMonitor, i.e.onlineMetaDataDigis, already defaults to the intended collection; at the same time,hltOnlineMetaDataDigisshould in principle be included in theDQMHistogramsEndPath in the HLT menus.)PR validation:
A local test confirms that the plots in question (profile vs PU, vs lumi) now have entries (see
).
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:
CMSSW_12_4_XCMSSW_12_5_X