Pack triggerobject standalone and photons/muon updated packing for miniaod #18739

arizzi · 2017-05-15T14:50:08Z

This PR packs further the TriggerObjectStandalone for miniaod (from 6Kb/ev on ttbar to 3.8kb/ev) as well as photon and muon objects.

The label packing in trigger object is now thread-safe and can be eventually further improved once the new function EventBase::getParameterSet is integrated in the release.

We could merge this as-is and then improve the access via the new function once available

@gpetruc @Dr15Jones

cmsbuild · 2017-05-15T14:50:37Z

A new Pull Request was created by @arizzi for master.

It involves the following packages:

DataFormats/PatCandidates
PhysicsTools/PatAlgos

@perrotta, @cmsbuild, @slava77, @monttj, @davidlange6 can you please review it and eventually sign? Thanks.
@TaiSakuma, @gouskos, @JyothsnaKomaragiri, @imarches, @ahinzmann, @acaudron, @mmarionncern, @rappoccio, @jdolen, @nhanvtran, @gpetruc, @gkasieczka, @schoef, @ferencek, @mverzett, @mariadalfonso, @pvmulder, @cbernet this is something you requested to watch as well.
@davidlange6 you are the release manager for this.

cms-bot commands are listed here

slava77 · 2017-05-15T15:53:37Z

@cmsbuild please test

cmsbuild · 2017-05-15T15:53:52Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/19839/console Started: 2017/05/15 21:25

Dr15Jones · 2017-05-15T15:15:51Z

DataFormats/PatCandidates/src/TriggerObjectStandAlone.cc

@@ -25,6 +31,7 @@ TriggerObjectStandAlone::TriggerObjectStandAlone() :
  TriggerObject()
 {
  filterLabels_.clear();
+  filterLabelIndices_.clear();


Why are you calling clear in a constructor?

Dr15Jones · 2017-05-15T15:17:45Z

DataFormats/PatCandidates/interface/TriggerObjectStandAlone.h

-      /// Vector of labels of all HLT filters or names od L1 conditions the trigger objects has been used in
-      std::vector< std::string > filterLabels_;
+      /// Vector of labels of all HLT filters or names of L1 conditions the trigger objects has been used in
+      mutable std::vector< std::string > filterLabels_;


This is not thread safe and therefore could not be put into the Event.

See the section on lazy caching for how to do this safely

https://twiki.cern.ch/twiki/bin/view/CMSPublic/FWMultithreadedThreadSafeDataStructures#Use_Atomics

I do not see a classes_def.xml rule that prevents this from being written out. So if it gets filled in someones job they would pay the storage cost.

well it is cleared before when packing, anyhow this topic of what we should write and when we should compress (what we should make mutable etc. etc..) is still to be finalized.

Dr15Jones · 2017-05-15T15:22:36Z

DataFormats/PatCandidates/src/TriggerObjectStandAlone.cc

+
+      //if (0!=(pset=psetRegistry->getMapped(config.parameterSetID()))) {
+
+      edm::pset::Registry* psetRegistry = edm::pset::Registry::instance();


This will almost certainly fail if someone attempts to use TTree::Draw since the registry will not have been filled. I also do not know if fwlite::Event calls would have filled the Registry.

Dr15Jones · 2017-05-15T15:25:51Z

DataFormats/PatCandidates/interface/TriggerObjectStandAlone.h

@@ -49,6 +52,9 @@ namespace pat {
      /// The vector is empty for data (size 0), if the according information is not available.
      std::vector< bool > pathL3FilterAccepted_;

+      edm::ParameterSetID  psetId_;
+      std::string processName_;


What is the point of processName_? It is set but can not be accessed and nothing reads it.

Dr15Jones · 2017-05-15T15:30:55Z

DataFormats/PatCandidates/src/TriggerObjectStandAlone.cc

@@ -306,3 +328,120 @@ void TriggerObjectStandAlone::unpackPathNames(const edm::TriggerNames &names) {
    pathNames_.swap(paths);
 }

+void TriggerObjectStandAlone::packFilterLabels(const std::vector<std::string> &names) const {
+    if (!filterLabelIndices_.empty()) {


This function is not thread safe

Dr15Jones · 2017-05-15T15:42:58Z

DataFormats/PatCandidates/src/TriggerObjectStandAlone.cc

@@ -260,6 +274,14 @@ bool TriggerObjectStandAlone::checkIfPathsAreUnpacked(bool throwIfPacked) const
   if (!unpacked && throwIfPacked) throw cms::Exception("RuntimeError", "This TriggerObjectStandAlone object has packed trigger path names. Before accessing path names you must call unpackPathNames with an edm::TriggerNames object. You can get the latter from the edm::Event or fwlite::Event and the TriggerResults\n");
   return unpacked;
 }
+bool TriggerObjectStandAlone::checkIfFiltersAreUnpacked(bool unpack) const {
+   bool unpacked = filterLabelIndices_.empty();


This is not thread safe since filterLabelIndices_ can mutate during a const call.

Dr15Jones · 2017-05-15T15:43:27Z

DataFormats/PatCandidates/src/TriggerObjectStandAlone.cc

@@ -260,6 +274,14 @@ bool TriggerObjectStandAlone::checkIfPathsAreUnpacked(bool throwIfPacked) const
   if (!unpacked && throwIfPacked) throw cms::Exception("RuntimeError", "This TriggerObjectStandAlone object has packed trigger path names. Before accessing path names you must call unpackPathNames with an edm::TriggerNames object. You can get the latter from the edm::Event or fwlite::Event and the TriggerResults\n");
   return unpacked;
 }
+bool TriggerObjectStandAlone::checkIfFiltersAreUnpacked(bool unpack) const {
+   bool unpacked = filterLabelIndices_.empty();


This is not thread safe since filterLabelIndices_ can mutate during a const call.

Dr15Jones · 2017-05-15T15:45:43Z

DataFormats/PatCandidates/interface/TriggerObjectStandAlone.h

-      std::vector< std::string > filterLabels_;
+      /// Vector of labels of all HLT filters or names of L1 conditions the trigger objects has been used in
+      mutable std::vector< std::string > filterLabels_;
+      mutable std::vector< uint16_t >    filterLabelIndices_; 


What is the intended relationship between filterLabels_ and filterLabelIndices_? On a quick look, it appears you only intend one of these to be filled at a time?

the original design was to have both possibly filled with the first being a fallback when the name cannot be mapped and at the same time acting as the "unpacked" container.
probably we do not need this flexibility and we can stick to one mutable and one persistent but a decision has not been made yet.

Dr15Jones · 2017-05-15T15:57:52Z

DataFormats/PatCandidates/src/TriggerObjectStandAlone.cc

+
+ 	const unsigned int n(triggerNames.size());
+	std::set<std::string> saveTags;
+   	for (unsigned int i=0;i!=n; ++i) {


I'm afraid I do not know what this code is doing.

base on
https://github.com/cms-sw/cmssw/blob/master/HLTrigger/HLTcore/src/HLTConfigData.cc#L107

Dr15Jones · 2017-05-15T16:07:20Z

DataFormats/PatCandidates/src/TriggerObjectStandAlone.cc

+   	using namespace trigger;
+
+   	const ParameterSet& HLTPSet(pset->getParameterSet("@trigger_paths"));
+   	auto   triggerNames= HLTPSet.getParameter<vector<string> >("@trigger_paths");


This doesn't seem to match what I see in edm::EventBase::triggerNames_ and edm::TriggerNames::TriggerNames(edm::ParameterSet cons&).

edm::pset::Registry* psetRegistry = edm::pset::Registry::instance(); edm::ParameterSet const* pset=0; if (0!=(pset=psetRegistry->getMapped(triggerResults.parameterSetID()))) { if (pset->existsAs<std::vector<std::string> >("@trigger_paths", true)) { TriggerNames triggerNames(*pset); ...

and the constructor does

TriggerNames::TriggerNames(edm::ParameterSet const& pset) { triggerNames_ = pset.getParameter<Strings>("@trigger_paths"); ...

So I think what you want is

auto triggerNames=pset->getParameter<std::vector<std::string>>("@trigger_paths");

this is taken from here:
https://github.com/cms-sw/cmssw/blob/master/HLTrigger/HLTcore/src/HLTConfigData.cc#L91-L95

Your code fragment is correct. What I originally looked at was starting from the PSetID for the trigger results PSet while your code is starting from the top level PSet.

cmsbuild · 2017-05-15T22:10:08Z

+1
Tested at: 3c82701
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18739/19839/summary.html

cmsbuild · 2017-05-15T22:10:12Z

Comparison job queued.

arizzi · 2017-05-15T22:30:08Z

The mutable are just a temporary fix before we decide the final way to go (i.e. allow both version of the object with packed and unpacked or force one of the two to be only transient)

The code in eventBase accesses triggerNames and this is NOT what we need here.
Here we want the module names (see code in HLTConfigData class)...otherwise I would just use Event::triggerNames()!!

Can you comment on how we can be sure that fwlite loads the registry? Is there a way to force it to do so?
In fact while can't we have a proper (api) way to access the provenance in fwlite?

cmsbuild · 2017-05-16T00:50:30Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18739/19839/summary.html

Comparison Summary:

You potentially added 3522 lines to the logs
ROOTFileChecks: Some differences in event products or their sizes found
Reco comparison results: 1723 differences found in the comparisons
DQMHistoTests: Total files compared: 24
DQMHistoTests: Total histograms compared: 1829394
DQMHistoTests: Total failures: 27216
DQMHistoTests: Total nulls: 39
DQMHistoTests: Total successes: 1801959
DQMHistoTests: Total skipped: 180
DQMHistoTests: Total Missing objects: 0
Checked 98 log files, 14 edm output root files, 24 DQM output files

arizzi · 2017-05-16T15:29:12Z

@Dr15Jones we are adapting it (I'll push the commit asap) to get the Event as input so that we are sure it internally calls triggerNames and hence the registry is filled... but... wouldn't it be better if EventBase has some
EventBase::parameterSet(psetId)
or even
EventBase::registry()
function that is properly implemented in fwlite::Event and edm::Event ?

Dr15Jones · 2017-05-16T15:30:02Z

I'm actually working on EventBase::parameterSet(psetId) right now :).

cmsbuild · 2017-05-26T14:48:05Z

Pull request #18739 was updated. @perrotta, @monttj, @cmsbuild, @franzoni, @slava77, @davidlange6 can you please check and sign again.

arizzi · 2017-05-26T14:48:37Z

@slava77 all set from our side, please trigger the compile and test again

perrotta · 2017-05-26T14:51:18Z

please test

cmsbuild · 2017-05-26T14:51:35Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/20143/console Started: 2017/05/26 16:52

slava77 · 2017-05-26T16:01:46Z

@arizzi
I'm missing an answer to #18739 (comment) about the PATPhotonProducer

arizzi · 2017-05-26T16:56:51Z

@gpetruc can you have a look?

slava77 · 2017-05-26T23:56:41Z

@cmsbuild please test

the last attempt failed in the infrastructure somewhere

ssl.SSLError: ('The read operation timed out',)
...
Finished: FAILURE

cmsbuild · 2017-05-26T23:56:58Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/20148/console Started: 2017/05/27 01:57

cmsbuild · 2017-05-27T02:52:37Z

+1
Tested at: 163fdb6
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18739/20148/summary.html

cmsbuild · 2017-05-27T02:52:40Z

Comparison job queued.

cmsbuild · 2017-05-27T07:53:00Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18739/20148/summary.html

Comparison Summary:

No significant changes to the logs found
ROOTFileChecks: Some differences in event products or their sizes found
Reco comparison results: 3359 differences found in the comparisons
DQMHistoTests: Total files compared: 24
DQMHistoTests: Total histograms compared: 1833626
DQMHistoTests: Total failures: 19544
DQMHistoTests: Total nulls: 65
DQMHistoTests: Total successes: 1813837
DQMHistoTests: Total skipped: 180
DQMHistoTests: Total Missing objects: 0
Checked 98 log files, 14 edm output root files, 24 DQM output files

arizzi · 2017-05-29T09:26:15Z

@slava77 anything else missing?

slava77 · 2017-05-29T12:13:51Z

On 5/29/17 2:26 AM, arizzi wrote: @slava77 <https://github.com/slava77> anything else missing?

I think no, I will check later today.

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18739 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEdcbkuLaxQoBbrvsw9YuV_gJh782x4Jks5r-o84gaJpZM4NbPn7>.

slava77 · 2017-05-31T17:41:27Z

+1

for #18739 163fdb6

code changes are in line with the description: modifications are in miniAOD.
jenkins tests pass and comparisons with baseline show differences only in miniAOD photons (zeroed-out values) and muons (round-offs introduce small bin-to-bin shifts in a few places)
local tests with more events confirm the expected behaviour:
- significant reduction in the trigger data from 10.6K to 7.2K per event using 200 events in 136.761; about 4% reduction in size of slimmedMuons (3K events in ZMM PU from wf 10242) and about 12% reduction in size in slimmedPhotons (3K events in Hgg PU from wf 10248).
- CPU changes are somewhat negligible: slimmedPatTrigger takes 1.2 ms/evt compared to 1.5 s/evt spent in miniAOD processing total.

gpetruc and others added 6 commits April 24, 2017 15:48

Slimming of trigger objects

1dbf3c0

Some muon slimming

8a4896a

Some photon slimming

36f8d6d

Merged slimming_4andrea from repository gpetruc

abc9c4a

add static member to access all module names

99b0495

take label list from provenance

3c82701

cmsbuild added this to the CMSSW_9_2_X milestone May 15, 2017

cmsbuild added analysis-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels May 15, 2017

cmsbuild added tests-started and removed tests-pending labels May 15, 2017

Dr15Jones suggested changes May 15, 2017

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels May 15, 2017

cmsbuild added comparison-available and removed comparison-pending labels May 16, 2017

remove transient members and force access via edm/fwlite::Event

daa666e

cmsbuild added tests-started and removed tests-pending labels May 26, 2017

cmsbuild added tests-pending and removed tests-started labels May 26, 2017

cmsbuild added tests-started and removed tests-pending labels May 26, 2017

cmsbuild added tests-approved and removed tests-started labels May 27, 2017

cmsbuild added comparison-available and removed comparison-pending labels May 27, 2017

cmsbuild added reconstruction-approved and removed reconstruction-pending labels May 31, 2017

davidlange6 merged commit 8b17758 into cms-sw:master Jun 1, 2017

gpetruc mentioned this pull request Jun 13, 2017

Fix decoding of filter labels #19211

Merged

arizzi deleted the triggerLabels910pre3 branch October 11, 2017 08:55

silviodonato mentioned this pull request Apr 27, 2020

Improvements of ReduceMantissaToNbitsRounding #29564

Open


		//if (0!=(pset=psetRegistry->getMapped(config.parameterSetID()))) {

		edm::pset::Registry* psetRegistry = edm::pset::Registry::instance();

Pack triggerobject standalone and photons/muon updated packing for miniaod #18739

Pack triggerobject standalone and photons/muon updated packing for miniaod #18739

Conversation

arizzi commented May 15, 2017 • edited

cmsbuild commented May 15, 2017

slava77 commented May 15, 2017

cmsbuild commented May 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented May 15, 2017

cmsbuild commented May 15, 2017

arizzi commented May 15, 2017

cmsbuild commented May 16, 2017

arizzi commented May 16, 2017

Dr15Jones commented May 16, 2017

cmsbuild commented May 26, 2017

arizzi commented May 26, 2017

perrotta commented May 26, 2017

cmsbuild commented May 26, 2017 • edited

slava77 commented May 26, 2017

arizzi commented May 26, 2017

slava77 commented May 26, 2017

cmsbuild commented May 26, 2017 • edited

cmsbuild commented May 27, 2017

cmsbuild commented May 27, 2017

cmsbuild commented May 27, 2017

arizzi commented May 29, 2017

slava77 commented May 29, 2017 via email

slava77 commented May 31, 2017

arizzi commented May 15, 2017 •

edited

cmsbuild commented May 15, 2017 •

edited

cmsbuild commented May 26, 2017 •

edited

cmsbuild commented May 26, 2017 •

edited