saving the collections of ECAL selective readout flags in pi0/eta stream output #20973

cippy · 2017-10-19T17:30:23Z

We are adding the collection of ECAL Selective Readout (SR) flags in the output of the pi0/eta stream.
These flags are needed to study the impact of the SR thresholds on the pi0/eta reconstruction from low energy photons.

The change affects

HLTrigger/special/src/HLTRechitsToDigis.cc
HLTrigger/special/BuildFile.xml
(the HLT configuration for the pi0/eta stream paths will have to be modified as well adding some parameters)

The source file implements the producer that saves the digis collection in the stream output (only those digis matched to rechits used to build pi0/eta are saved). Similarly, the change we implemented takes the SrFlags collection and produce a new collections storing only the flags matched to digis that are finally kept in the stream output.

The BuildFile.xml must have a new line to include a header used by the source.

Tested from release CMSSW_9_4_0_pre2 running the (updated) HLT configuration on HLTPhysics dataset. The new collections are created and stored in the output dataset.
The event size changes by 2% from 1.96 kB/event to 1.99 kB/event

cmsbuild · 2017-10-19T17:32:13Z

The code-checks are being triggered in jenkins.

cmsbuild · 2017-10-19T17:34:48Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20973/1558

cmsbuild · 2017-10-19T17:36:10Z

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

It involves the following packages:

HLTrigger/special

@Martin-Grunewald, @silviodonato, @cmsbuild, @fwyzard can you please review it and eventually sign? Thanks.
@Martin-Grunewald this is something you requested to watch as well.
@davidlange6, @slava77 you are the release manager for this.

cms-bot commands are listed here

Martin-Grunewald · 2017-10-20T07:09:39Z

Please change the title of the PR to something meaningful.

Also, in case the label of the new InputTag is empty, revert to the old behavior.
The default in fillDescriptions for the new InputTag should thus be an empty InputTag.
This way the PR will not crash when running on current HLT py configs.

cippy · 2017-10-20T13:57:59Z

@emanueledimarco

@Martin-Grunewald
I changed the name of the issue

However,I'm not sure I understand the second point.
Should I change

desc.addedm::InputTag("srFlagsIn",edm::InputTag("ecalDigis"))
with
desc.addedm::InputTag("srFlagsIn",edm::InputTag())
?
Note that the input collection has always existed, it is produced by the ECALRawToDigi producer called as the first step of the stream, but was not saved.
What is new in this development is the fact the the (pruned) collection is also stored in the stream output (we could have just used a 'keep' in the outputModule, but we wanted to prune the collection because we only need the flags in some regions)

Martin-Grunewald · 2017-10-20T14:02:58Z

My comment is about the (technical) backward compatibility of the code changes to deal with exisiting py configs but where the code base includes this PR. The fillDescriptions value provided is then inserted/used, and that should tell the module to behave in the old way.
This way we can integrate the code already now, and do the py config update of the HLT menu later (after parsing into ConfDB) without having to do both at the same time.

Martin-Grunewald · 2017-10-20T14:06:09Z

And yes, "ecalDigis" does not exist at HLT where all module labels start with hlt.

cippy · 2017-10-20T14:26:50Z

I see, so would it be ok to have instead
desc.addedm::InputTag("srFlagsIn",edm::InputTag("hltEcalDigis"))
?
Indeed in my HLT configuration I see "hltEcalDigis" is the name used also in other producers to get the same collection. I used ecalDigis before because that was appearing in the fillDescriptions for other parameters.
If this is enough I will commit the new change

Martin-Grunewald · 2017-10-20T14:27:39Z

No, Empty and test in the code!

cippy · 2017-10-20T15:03:05Z

Hi,
If I use
desc.addedm::InputTag("srFlagsIn",edm::InputTag())
and run the old configuration, where the new parameters are not yet defined,
I get this error [0]

On the other hand, if I use
desc.addedm::InputTag("srFlagsIn",edm::InputTag("hltEcalDigis"))
the old configuration runs fine and the output will have the new collection, with the caveat that the label for the new output is always the same one (as in the fillDescriptions), while at some point the new parameter will assign different names depending on the module name (see here [1] extracted from edmDumpEventContent)

[0]
----- Begin Fatal Exception 20-Oct-2017 16:49:46 CEST-----------------------
An exception of category 'ProductNotFound' occurred while
[0] Processing Event run: 297113 lumi: 236 event: 440179434 stream: 1
[1] Running path 'AlCa_EcalPi0EBonly_v11'
[2] Calling method for module HLTRechitsToDigis/'hltAlCaPi0EBRechitsToDigis'
Exception Message:
Principal::getByToken: Found zero products matching all criteria
Looking for type: edm::SortedCollection<EBSrFlag,edm::StrictWeakOrdering >
Looking for module label: @EmptyLabel@
Looking for productInstanceName:

[1]
Type Module Label Process

edm::SortedCollection<EBSrFlag,edm::StrictWeakOrdering > "hltAlCaEtaEBRechitsToDigis" "pi0EBSrFlags" "MYHLT"
edm::SortedCollection<EBSrFlag,edm::StrictWeakOrdering > "hltAlCaPi0EBRechitsToDigis" "pi0EBSrFlags" "MYHLT"
edm::SortedCollection<EESrFlag,edm::StrictWeakOrdering > "hltAlCaEtaEERechitsToDigis" "pi0EBSrFlags" "MYHLT"
edm::SortedCollection<EESrFlag,edm::StrictWeakOrdering > "hltAlCaPi0EERechitsToDigis" "pi0EBSrFlags" "MYHLT"
e

cippy · 2017-10-20T15:11:41Z

The solution [0] would probably work if in the product() method I added something like,
if (not inputTag is empy) {
read the new input, fill the new ...
}

emanueledimarco · 2017-10-20T15:12:43Z

@Martin-Grunewald what is the need of integrating the C++ before the update of the HLT menu?
If the menu is updated consistently it will run w/o the need of checking wether the inputtag is filled or not.

Martin-Grunewald · 2017-10-20T15:20:20Z

@cippy Yes, exactly, that's what I mean the code should reproduce the old behaviour (ie, does not do the new stuff at all).
@emanueledimarco Flexibility in integration and deployment of the new features while keeping all old configs working properly with the new code.
(We want to avoid changing the configs of the frozen menus dumped into CMSSW).

Martin-Grunewald · 2017-10-20T15:21:53Z

https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideGlobalHLT#Trigger_development_for_Run_2

especially

Specifically note that the current trigger development release is 92X. Thus, for any new C++ code 
needed for your development, you need to provide a pull request for 94X first and then a backport PR 
for 92X. For added module parameters and functionality, you must use either a fillDescriptions(.) 
method (preferred) or existsAs (deprecated) to take care of older py configs not yet containing the 
new parameters. The new parameters' default values must be set in such a way so that the old 
behaviour of the module is reproduced! Also, the result of the code must not depend on whether any 
parameter is present in the py config or not, only on its default value (in fillDescriptions) or its value in 
the py config! This allows us to integrate code developments independently (and earlier) than updating 
the HLT menu (python config) itself with your changes: the python update requires ConfDB parsing to 
discover the new parameters, followed by a migration of the HLT menu within ConfDB to the newly 
parsed code template.

…InputTag is empty (default)

cmsbuild · 2017-10-20T23:08:17Z

The code-checks are being triggered in jenkins.

cmsbuild · 2017-10-20T23:23:35Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20973/1582

cmsbuild · 2017-10-20T23:23:55Z

Pull request #20973 was updated. @Martin-Grunewald, @silviodonato, @cmsbuild, @fwyzard can you please check and sign again.

Martin-Grunewald · 2017-10-21T08:03:10Z

We only need 92X and 94X.

Martin-Grunewald · 2017-10-21T08:03:19Z

please test

cmsbuild · 2017-10-21T08:03:28Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/23908/console Started: 2017/10/21 10:03

cmsbuild · 2017-10-21T11:04:53Z

+1
Tested at: 7515b14
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20973/23908/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
e887141
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20973/23908/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20973/23908/git-merge-result

cmsbuild · 2017-10-21T11:04:56Z

Comparison job queued.

cmsbuild · 2017-10-21T12:59:19Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 26
DQMHistoTests: Total histograms compared: 2795796
DQMHistoTests: Total failures: 105
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2795520
DQMHistoTests: Total skipped: 171
DQMHistoTests: Total Missing objects: 0
Checked 107 log files, 10 edm output root files, 26 DQM output files

Martin-Grunewald · 2017-10-24T08:54:57Z

+1

cmsbuild · 2017-10-24T08:55:58Z

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. @davidlange6, @slava77, @smuzaffar (and backports should be raised in the release meeting by the corresponding L2)

davidlange6 · 2017-10-25T08:19:44Z

+1

cippy added 3 commits October 18, 2017 11:47

modified to save SrFlags collections, but only flags matched to digis

961b1ff

fix in fillDescriptions

d0413ae

another fix in fillDescriptions

0616655

cmsbuild added this to the CMSSW_9_4_X milestone Oct 19, 2017

cmsbuild added code-checks-pending comparison-pending hlt-pending orp-pending pending-signatures tests-pending labels Oct 19, 2017

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 19, 2017

cippy mentioned this pull request Oct 19, 2017

saving the collections of ECAL selective readout flags (SrFlags) matc… #20974

Merged

cippy changed the title ~~Hltdev 9 4 0pre2 mciprian~~ saving the collections of ECAL selective readout flags in pi0/eta stream output Oct 20, 2017

fix for backward compatibility: do not save SrFlags if corresponding …

f3e62bf

…InputTag is empty (default)

cmsbuild removed the code-checks-approved label Oct 20, 2017

using nullptr instead of 0 to declare a null pointer

7515b14

cmsbuild added code-checks-pending and removed code-checks-rejected labels Oct 20, 2017

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 20, 2017

cmsbuild added tests-started and removed tests-pending labels Oct 21, 2017

cmsbuild added tests-approved and removed tests-started labels Oct 21, 2017

cmsbuild added comparison-available and removed comparison-pending labels Oct 21, 2017

cmsbuild added fully-signed hlt-approved and removed hlt-pending pending-signatures labels Oct 24, 2017

cmsbuild added orp-approved and removed orp-pending labels Oct 25, 2017

cmsbuild merged commit 803bf44 into cms-sw:master Oct 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

saving the collections of ECAL selective readout flags in pi0/eta stream output #20973

saving the collections of ECAL selective readout flags in pi0/eta stream output #20973

cippy commented Oct 19, 2017

cmsbuild commented Oct 19, 2017

cmsbuild commented Oct 19, 2017

cmsbuild commented Oct 19, 2017

Martin-Grunewald commented Oct 20, 2017

cippy commented Oct 20, 2017

Martin-Grunewald commented Oct 20, 2017 •

edited

Martin-Grunewald commented Oct 20, 2017

cippy commented Oct 20, 2017

Martin-Grunewald commented Oct 20, 2017

cippy commented Oct 20, 2017

cippy commented Oct 20, 2017 •

edited

emanueledimarco commented Oct 20, 2017

Martin-Grunewald commented Oct 20, 2017 •

edited

Martin-Grunewald commented Oct 20, 2017 •

edited

cmsbuild commented Oct 20, 2017

cmsbuild commented Oct 20, 2017

cmsbuild commented Oct 20, 2017

Martin-Grunewald commented Oct 21, 2017

Martin-Grunewald commented Oct 21, 2017

cmsbuild commented Oct 21, 2017 •

edited

cmsbuild commented Oct 21, 2017

cmsbuild commented Oct 21, 2017

cmsbuild commented Oct 21, 2017

Martin-Grunewald commented Oct 24, 2017

cmsbuild commented Oct 24, 2017

davidlange6 commented Oct 25, 2017

saving the collections of ECAL selective readout flags in pi0/eta stream output #20973

saving the collections of ECAL selective readout flags in pi0/eta stream output #20973

Conversation

cippy commented Oct 19, 2017

cmsbuild commented Oct 19, 2017

cmsbuild commented Oct 19, 2017

cmsbuild commented Oct 19, 2017

Martin-Grunewald commented Oct 20, 2017

cippy commented Oct 20, 2017

Martin-Grunewald commented Oct 20, 2017 • edited

Martin-Grunewald commented Oct 20, 2017

cippy commented Oct 20, 2017

Martin-Grunewald commented Oct 20, 2017

cippy commented Oct 20, 2017

[1] Type Module Label Process

cippy commented Oct 20, 2017 • edited

emanueledimarco commented Oct 20, 2017

Martin-Grunewald commented Oct 20, 2017 • edited

Martin-Grunewald commented Oct 20, 2017 • edited

cmsbuild commented Oct 20, 2017

cmsbuild commented Oct 20, 2017

cmsbuild commented Oct 20, 2017

Martin-Grunewald commented Oct 21, 2017

Martin-Grunewald commented Oct 21, 2017

cmsbuild commented Oct 21, 2017 • edited

cmsbuild commented Oct 21, 2017

cmsbuild commented Oct 21, 2017

cmsbuild commented Oct 21, 2017

Martin-Grunewald commented Oct 24, 2017

cmsbuild commented Oct 24, 2017

davidlange6 commented Oct 25, 2017

Martin-Grunewald commented Oct 20, 2017 •

edited

[1]
Type Module Label Process

cippy commented Oct 20, 2017 •

edited

Martin-Grunewald commented Oct 20, 2017 •

edited

Martin-Grunewald commented Oct 20, 2017 •

edited

cmsbuild commented Oct 21, 2017 •

edited