Skip to content
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

fix collection names for premixing #20221

Closed
wants to merge 1 commit into from
Closed

Conversation

deguio
Copy link
Contributor

@deguio deguio commented Aug 21, 2017

this PR fixes:

  • the name of the TP collection being packed into the rawDataCollection when the data mixing is used.
  • the name of the TP collection used in the validation code.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @deguio (Federico De Guio) for master.

It involves the following packages:

Configuration/StandardSequences
Validation/HcalDigis

@vazzolini, @kmaeshima, @dmitrijus, @cmsbuild, @franzoni, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @makortel, @felicepantaleo, @GiacomoSguazzoni, @rovere, @VinInn, @Martin-Grunewald, @ebrondol, @dgulhan this is something you requested to watch as well.
@davidlange6, @slava77 you are the release manager for this.

cms-bot commands are listed here

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20221/228

@deguio
Copy link
Contributor Author

deguio commented Aug 21, 2017

please test

1 similar comment
@kpedro88
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 21, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/22405/console Started: 2017/08/21 21:27

@cmsbuild
Copy link
Contributor

-1

Tested at: a2b7b4e

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

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20221/22405/summary.html

I found follow errors while testing this PR

Failed tests: RelVals AddOn

  • RelVals:

When I ran the RelVals I found an error in the following worklfows:
5.1 step1

runTheMatrix-results/5.1_TTbar+TTbarFS+HARVESTFS/step1_TTbar+TTbarFS+HARVESTFS.log

135.4 step1
runTheMatrix-results/135.4_ZEE_13+ZEEFS_13+HARVESTUP15FS+MINIAODMCUP15FS/step1_ZEE_13+ZEEFS_13+HARVESTUP15FS+MINIAODMCUP15FS.log

  • AddOn:

I found errors in the following addon tests:

cmsDriver.py TTbar_13TeV_TuneCUETP8M1_cfi --conditions auto:run2_mc --fast -n 100 --eventcontent AODSIM,DQM --relval 100000,1000 -s GEN,SIM,RECOBEFMIX,DIGI:pdigi_valid,L1,DIGI2RAW,L1Reco,RECO,EI,VALIDATION --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --datatier GEN-SIM-DIGI-RECO,DQMIO --beamspot NominalCollision2015 --era Run2_25ns : FAILED - time: date Tue Aug 22 00:05:30 2017-date Mon Aug 21 23:59:09 2017 s - exit: 17920
cmsDriver.py TTbar_13TeV_TuneCUETP8M1_cfi --conditions auto:run2_mc --fast -n 100 --eventcontent AODSIM,DQM --relval 100000,1000 -s GEN,SIM,RECOBEFMIX,DIGI:pdigi_valid,L1,DIGI2RAW,L1Reco,RECO,EI,VALIDATION --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --datatier GEN-SIM-DIGI-RECO,DQMIO --beamspot NominalCollision2015 --era Run2_2016 : FAILED - time: date Tue Aug 22 00:05:28 2017-date Mon Aug 21 23:59:20 2017 s - exit: 17920
cmsDriver.py TTbar_8TeV_TuneCUETP8M1_cfi --conditions auto:run1_mc --fast -n 100 --eventcontent AODSIM,DQM --relval 100000,1000 -s GEN,SIM,RECOBEFMIX,DIGI:pdigi_valid,L1,DIGI2RAW,L1Reco,RECO,EI,VALIDATION --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --datatier GEN-SIM-DIGI-RECO,DQMIO --beamspot Realistic8TeVCollision : FAILED - time: date Tue Aug 22 00:18:05 2017-date Tue Aug 22 00:05:37 2017 s - exit: 17920

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

@cmsbuild
Copy link
Contributor

Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped)

@abdoulline
Copy link

There is no packing/unpacking in Fastsim (means no hcalDigi) workflows...

<...>
Calling method for module HcalDigisValidation/'AllHcalDigisValidation'
Exception Message:
Principal::getByToken: Found zero products matching all criteria
Looking for type: edm::SortedCollection<HcalTriggerPrimitiveDigi,edm::StrictWeakOrdering >
Looking for module label: hcalDigis
<...>

(5.1)
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20221/22405/runTheMatrix-results/5.1_TTbar+TTbarFS+HARVESTFS/TTbar_8TeV_TuneCUETP8M1_cfi_GEN_SIM_RECOBEFMIX_DIGI_L1_DIGI2RAW_L1Reco_RECO_EI_VALIDATION_DQM.py

(134.1)
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20221/22405/runTheMatrix-results/135.4_ZEE_13+ZEEFS_13+HARVESTUP15FS+MINIAODMCUP15FS/ZEE_13TeV_TuneCUETP8M1_cfi_GEN_SIM_RECOBEFMIX_DIGI_L1_DIGI2RAW_L1Reco_RECO_EI_VALIDATION_DQM.py

@deguio
Copy link
Contributor Author

deguio commented Aug 22, 2017

sigh, I didn't think about that.
it means that we have to go for the more complicated solution that includes:

  • adding the DMHcalTriggerPrimitiveDigis to the event content
  • implement a customization function that reacts to the preMix option from cmsDriver
  • in that function pick the right collection for the validaiton module for the normal case (simHcalTriggerPrimitiveDigis) and the premix case (DMHcalTriggerPrimitiveDigis)

unless somebody has a smarter idea.

@abdoulline
Copy link

abdoulline commented Aug 22, 2017

@kpedro88, @deguio

I'm wondering if we can simply customize (reverting it for fastSim only)
http://cmslxr.fnal.gov/source/Validation/HcalDigis/python/HcalDigisParam_cfi.py

with:
from Configuration.Eras.Modifier_fastSim_cff import fastSim
fastSim.toModify(hcaldigisAnalyzer, hcaldigisAnalyzer.dataTPs = cms.InputTag("simHcalTriggerPrimitiveDigis"))

@kpedro88
Copy link
Contributor

@abdoulline that is one potential solution; I still want to hear from trigger experts (e.g. @christopheralanwest) if this change is desirable in general.

To clarify, the preferred more complicated solution would be:

  • make Configuration/StandardSequences/python/ValidationDM_cff.py that contains:
from Configuration.StandardSequences.Validation_cff import *
hcaldigisAnalyzer.dataTPs = cms.InputTag("DMHcalTriggerPrimitiveDigis")

@christopheralanwest
Copy link
Contributor

@kpedro88, @abdoulline: I think either solution would work.

However, I'm concerned about the change to using the unpacked TPs in the validation by default as I believe it is the simHcalTriggerPrimitiveDigis that are used downstream. In fact, before PR #19804, the packed TPs were not correct. A 92X backport of #19804 would be needed before PR #20222 (the backport for this PR) is merged, but I think that there was no such backport; perhaps @matz-e can confirm. Before switching to using unpacked TPs in the validation, it should be verified that the packed TPs are identical to those from the original collection.

@matz-e
Copy link
Contributor

matz-e commented Aug 23, 2017

There was no backport of #19804. I'm never sure which releases need backports by now. I can quickly submit a backport.

@abdoulline
Copy link

abdoulline commented Aug 23, 2017 via email

@dmitrijus
Copy link
Contributor

+1

@deguio
Copy link
Contributor Author

deguio commented Aug 23, 2017

closing while we decide what's the best solution to implement

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants