Do not update HepMCProduct in place (producer implementation) #10858

wmtan · 2015-08-19T16:45:36Z

This PR is the chosen implementation for avoiding modifying HepMCProduct in place (aka smearing the vertices in place). The smearers (aka vertex generators) still run as EDProducers, but they copy the HepMCProduct, smear the copy, and put the copy into the event with module label "VtxSmeared".
In order to make this PR backward compatible (see below) another producer with label "generatorSmeared"copies the HepMCProduct, and the HepMCProduct with label "generatorSmeared" is the one that is kept on output.
The consumers of the smeared HepMCProduct are modified to use the module label "generatorSmeared" instead of "generator". Also, the generators now create the HepMCProduct with the product instance name "unsmeared", so consumers of the unsmeared HepMCProduct must explicitly use "unsmeared" as the product instance name, in addition to the "generator" module label. This is a defensive measure to ensure that consumers always get the smeared HepMCProduct unless they explicitly specify "unsmeared".
For backward compatibility, the producer of HepMCProduct with label "generatorSmeared" also runs at the beginning of the digitization step. If it finds a HepMCproduct with label "generator" or "VtxSmeared", it copies it, creating a HepMCProduct with module label "generatorSmeared". In this way, files containing GEN-SIM written prior to this PR can still be read.

cmsbuild · 2015-08-19T16:46:21Z

A new Pull Request was created by @wmtan for CMSSW_7_6_X.

Do not update HepMCProduct in place (producer implementation)

It involves the following packages:

Alignment/LaserAlignmentSimulation
Calibration/HcalCalibAlgos
Calibration/IsolatedParticles
Configuration/EcalTB
Configuration/Generator
Configuration/StandardSequences
DPGAnalysis/SiStripTools
EgammaAnalysis/ElectronTools
ElectroWeakAnalysis/ZMuMu
FastSimulation/Configuration
FastSimulation/EventProducer
FastSimulation/ForwardDetectors
GeneratorInterface/AMPTInterface
GeneratorInterface/AlpgenInterface
GeneratorInterface/BeamHaloGenerator
GeneratorInterface/Configuration
GeneratorInterface/Core
GeneratorInterface/CosmicMuonGenerator
GeneratorInterface/ExternalDecays
GeneratorInterface/GenFilters
GeneratorInterface/Herwig6Interface
GeneratorInterface/HiGenCommon
GeneratorInterface/Hydjet2Interface
GeneratorInterface/HydjetInterface
GeneratorInterface/LHEInterface
GeneratorInterface/MCatNLOInterface
GeneratorInterface/PyquenInterface
GeneratorInterface/Pythia6Interface
GeneratorInterface/Pythia8Interface
GeneratorInterface/RivetInterface
GeneratorInterface/TauolaInterface
IOMC/EventVertexGenerators
IOMC/Input
IOMC/ParticleGuns
PhysicsTools/HepMCCandAlgos
PhysicsTools/PatAlgos
RecoEcal/EgammaCoreTools
RecoEgamma/Examples
RecoJets/JetAnalyzers
RecoParticleFlow/Configuration
RecoParticleFlow/PFProducer
SUSYBSMAnalysis/HSCP
SimCalorimetry/EcalTestBeam
SimDataFormats/GeneratorProducts
SimG4CMS/Calo
SimG4Core/Application
SimGeneral/MixingModule
SimGeneral/PileupInformation
SimGeneral/TrackingAnalysis
SimMuon/GEMDigitizer
SimMuon/RPCDigitizer
SimTracker/TrackHistory
SimTracker/TrackerMaterialAnalysis
SimTransport/HectorProducer
TauAnalysis/MCEmbeddingTools
Validation/CaloTowers
Validation/Configuration
Validation/EcalClusters
Validation/EcalDigis
Validation/EcalHits
Validation/EcalRecHits
Validation/EventGenerator
Validation/HcalHits
Validation/HcalRecHits
Validation/RecoEgamma
Validation/RecoHI
Validation/RecoJets
Validation/RecoParticleFlow
Validation/RecoVertex

@diguida, @lveldere, @cerminar, @covarell, @bendavid, @civanch, @cmsbuild, @davidlange6, @vciulli, @smuzaffar, @Dr15Jones, @cvuosalo, @mdhildreth, @deguio, @slava77, @vadler, @mmusich, @ssekmen, @danduggan, @thuer, @monttj, @franzoni can you please review it and eventually sign? Thanks.
@ghellwig, @TaiSakuma, @abbiendi, @rappoccio, @argiro, @Martin-Grunewald, @istaslis, @lgray, @threus, @venturia, @mmarionncern, @battibass, @makortel, @acaudron, @jhgoh, @dgulhan, @inugent, @jdolen, @ferencek, @cerati, @trocino, @Sam-Harper, @kkrajczar, @GiacomoSguazzoni, @rovere, @VinInn, @nhanvtran, @schoef, @mschrode, @richard-cms, @mkirsano, @imarches, @ahinzmann, @rylanC24, @gpetruc, @matt-komm, @mariadalfonso, @pvmulder, @cbaus, @bachtis this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@degano you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

slava77 · 2015-08-19T16:51:24Z

@cmsbuild please test

cmsbuild · 2015-08-19T16:56:19Z

The tests are being triggered in jenkins.

cmsbuild · 2015-08-19T21:02:44Z

-1

Tested at: 7a4f54f
I found errors in the following unit tests:

---> test runtestPhysicsToolsPatAlgos had ERRORS

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

wmtan · 2015-08-19T21:16:19Z

@cmsbuild @davidlange6 the unit test that fails also fails in the base IB. The failure is not caused by this PR.

cmsbuild · 2015-08-19T22:52:57Z

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

lveldere · 2015-08-20T08:23:00Z

+1

fastsim edits look ok,
differences observed in fastsim comparisons,
look compatible with fullsim comparisons.

cmsbuild · 2015-09-20T01:03:47Z

+1
Tested at: f719245
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-10858/8225/summary.html

cmsbuild · 2015-09-20T02:01:26Z

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

wmtan · 2015-09-22T19:18:41Z

@covarell, @bendavid, @govoni, @vciulli, @cvuosalo, @slava77, @vadler, @thuer, @monttj, @franzoni ping: analysis, generators, operations, reconstruction Can you either sign this, or provide a reason for not signing.

slava77 · 2015-09-23T17:52:40Z

GeneratorInterface/Core/interface/GeneratorSmearedProducer.h

+  class HepMCProduct;
+}
+
+class GeneratorSmearedProducer : public edm::EDProducer {


why is this one not thread-aware? (edm::one, stream )

Yes, this new producer, as well as the modified vertex smeared producer, should be thread aware (e.g. one, stream, or global), just as all other modules should be. That should happen in due course.

slava77 · 2015-09-23T18:01:18Z

+1

for #10858 f719245

code changes are mostly as expected
a newly introduced producer class GeneratorSmearedProducer : public edm::EDProducer should be changed to an edm::one, I suppose, but better do it in a separate PR to have less signatures to collect
jenkins tests pass and comparisons with the baseline show no differences

wmtan · 2015-09-24T16:28:56Z

@covarell, @bendavid, @govoni, @vciulli, @vadler, @thuer, @monttj, @franzoni
ping again: analysis, generators, operations Can you either sign this, or provide a reason for not signing?

Do not update HepMCProduct in place (producer implementation)

ferencek · 2015-10-08T05:48:48Z

@wmtan, this PR has broken the jet flavor code. In CMSSW_7_6_X_2015-10-07-2300 I now get a bunch of warnings like the following one

Begin processing the 500th record. Run 1, Event 1800, LumiSection 18 at 08-Oct-2015 00:32:46.939 CDT
%MSG-w UndefinedPartonMode:  HadronAndPartonSelector:patJetPartons  08-Oct-2015 00:32:46 CDT Run: 1 Event: 1800
Could not automatically determine the hadronizer type and set the correct parton selection mode. Parton-based jet flavour will not be defined.

when running PAT tests. How did this go unnoticed?

Anyways, the hadron and parton selector defined in PhysicsTools/JetMCAlgos/plugins/HadronAndPartonSelector.cc needs to access the GenEventInfoProduct in order to determine the hadronizer type. What is the proposed solution here. Is that product still available in the event and under what name?

wmtan · 2015-10-08T14:07:37Z

@ferencek This PR should not have affected the GenEventInfoProduct in any way.
Please provide evidence that this PR broke the jet flavor code.
At a minimum, I would need a cmsRun job that reproduces this error.
I cannot provide a solution to a problem that I do not understand, and that, at least on the surface, appears to have nothing whatever to do with this PR.

ferencek · 2015-10-08T15:26:11Z

@wmtan, evidence is the printout I provided above. The problem can be reproduced by runnning pretty much any of the PAT unit tests. For instance, running PhysicsTools/PatAlgos/test/patTuple_standard_cfg.py should suffice.

The problem originates from the change made in PhysicsTools/PatAlgos/python/mcMatchLayer0/jetFlavourId_cff.py. If the GenEventInfoProduct is unaffected, then changing

src = cms.InputTag("generatorSmeared"),

back to

src = cms.InputTag("generator"),

should fix the issue.

wmtan · 2015-10-08T17:13:08Z

@ferencek 1) The reason this problem was not found is that it does not cause any tests to fail. The PR approval process does not scan all output files for new warning messages.
2) I now understand the problem. It is only a backward compatibility issue. GenEventInfoProduct is indeed unaffected. Changing "generatorSmeared" back to "generator" does indeed fix the unit test, but only because the file being read predates this PR. Once a new input file for this test is generated, it would have to be changed back to "generatorSmeared".
3) Backward compatibility was indeed addressed in this PR, but the fix does not cover the unusual workflow in this test.
4) Further discussion should be in e-mail outside of github, unless it is very short. If you want a record of the further discussion, use an appropriate HN forum.

ferencek · 2015-10-08T20:58:46Z

@wmtan, ok, it sounds like the problem will be fixed once the input RelVals are updated to a version that contains this PR. In that case, I don't think any fixes are needed.

wmtan · 2015-10-08T21:23:06Z

@ferencek That is correct, assuming that the relvals in question smear the vertices in HepMCProduct, which they should.

ferencek · 2015-10-23T01:08:18Z

@wmtan, I had a look at one of the files from /RelValProdTTbar_13/CMSSW_7_6_0_pre7-76X_mcRun2_asymptotic_v5-v1/AODSIM and here is what I see

[ferencek@cmslpc25 src]$ edmDumpEventContent root://cmsxrootd-site.fnal.gov//store/relval/CMSSW_7_6_0_pre7/RelValProdTTbar_13/AODSIM/76X_mcRun2_asymptotic_v5-v1/00000/0E9A5DE8-1D71-E511-A205-00261894380D.root
Type                                  Module                      Label             Process   
----------------------------------------------------------------------------------------------
GenEventInfoProduct                   "generator"                 ""                "SIM"     
edm::TriggerResults                   "TriggerResults"            ""                "SIM"     
edm::TriggerResults                   "TriggerResults"            ""                "HLT"     
int                                   "addPileupInfo"             "bunchSpacing"    "HLT"     
vector<PileupSummaryInfo>             "addPileupInfo"             ""                "HLT"     
vector<int>                           "genParticles"              ""                "HLT"
...

and

[ferencek@cmslpc25 src]$ edmProvDump  root://cmsxrootd-site.fnal.gov//store/relval/CMSSW_7_6_0_pre7/RelValProdTTbar_13/AODSIM/76X_mcRun2_asymptotic_v5-v1/00000/0E9A5DE8-1D71-E511-A205-00261894380D.root | grep -i cmssw
  SIM '' '"CMSSW_7_6_0_pre7"' (6459b08dbcc3c6e5421bfc57cfa6cb04)
    HLT '' '"CMSSW_7_6_0_pre7"' (e355911c26dddb8d43fe6d5bca5bbf21)
      RECO '' '"CMSSW_7_6_0_pre7"' (39f973222d399b2f7e7caed12b632381)
  EfficiencyMap: string tracked  = 'CondFormats/JetMETObjects/data/CMSSW_538_TrackNonEff.txt'
  LeakageMap: string tracked  = 'CondFormats/JetMETObjects/data/CMSSW_538_TrackLeakage.txt'
  ResponseMap: string tracked  = 'CondFormats/JetMETObjects/data/CMSSW_538_response.txt'
  tagName: vstring tracked  = {'ZSP_CMSSW390_Akt_05_PU0'}
   FileName: FileInPath tracked  = V001 SimG4CMS/Forward/data/CastorShowerLibrary_CMSSW500_Standard.root 2 /external/slc6_amd64_gcc493/data/SimG4CMS/Forward/data/CastorShowerLibrary_CMSSW500_Standard.root

and I don't see any GenEventInfoProducts called "generatorSmeared". I'm not sure if that was expected but if so, then the changes made in PhysicsTools/PatAlgos/python/mcMatchLayer0/jetFlavourId_cff.py should be reverted.

wmtan · 2015-10-23T14:50:03Z

@ferencek The label "generatorSmeared" only applies to HepMCProduct, not to GenEventInfoProduct.
So, nothing is wrong here.

ferencek · 2015-10-23T16:39:49Z

@wmtan, thanks for the reply. Then my comment stands. The change done in PhysicsTools/PatAlgos/python/mcMatchLayer0/jetFlavourId_cff.py should be reverted since the modified parameter does not refer to a HepMCProduct but a GenEventInfoProduct. I can make a PR for that if you agree that this is what should be done. Thanks

wmtan · 2015-10-23T16:45:40Z

@ferencek Yes, you are right. It does refer to a GenEventInfoProduct. The change to that one file should be reverted.

cmsbuild added this to the Next CMSSW_7_6_X milestone Aug 19, 2015

cmsbuild added reconstruction-pending simulation-pending dqm-pending core-pending analysis-pending alca-pending fastsim-pending generators-pending operations-pending pending-signatures tests-pending orp-pending comparison-pending labels Aug 19, 2015

wmtan mentioned this pull request Aug 19, 2015

Do not modify HepMCProduct in place. It's not thread safe. #10610

Closed

cmsbuild added tests-started and removed tests-pending labels Aug 19, 2015

cmsbuild added tests-rejected and removed tests-started labels Aug 19, 2015

cmsbuild added comparison-available and removed comparison-pending labels Aug 19, 2015

cmsbuild added fastsim-approved and removed fastsim-pending labels Aug 20, 2015

cmsbuild added the tests-started label Sep 19, 2015

cmsbuild added tests-approved and removed tests-started labels Sep 20, 2015

cmsbuild added comparison-available and removed comparison-pending labels Sep 20, 2015

slava77 reviewed Sep 23, 2015
View reviewed changes

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Sep 23, 2015

davidlange6 added a commit that referenced this pull request Sep 25, 2015

Merge pull request #10858 from wmtan/VtxSmeared

46011a7

Do not update HepMCProduct in place (producer implementation)

davidlange6 merged commit 46011a7 into cms-sw:CMSSW_7_6_X Sep 25, 2015

wmtan mentioned this pull request Oct 5, 2015

Add HepMCProduct backward compatibility for premixing #11639

Merged

wmtan deleted the VtxSmeared branch October 6, 2015 17:14

diguida mentioned this pull request Oct 13, 2015

bsunanda:Run2-alca18 Restore Olgas code and modify the others to do both NZS and iterative analysis #10176

Merged

ferencek mentioned this pull request Oct 23, 2015

Quieting 76x step2 and step3 #11963

Merged

ferencek mentioned this pull request Oct 23, 2015

Reverting the src parameter value for PAT jet partons #12080

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not update HepMCProduct in place (producer implementation) #10858

Do not update HepMCProduct in place (producer implementation) #10858

wmtan commented Aug 19, 2015

cmsbuild commented Aug 19, 2015

slava77 commented Aug 19, 2015

cmsbuild commented Aug 19, 2015

cmsbuild commented Aug 19, 2015

wmtan commented Aug 19, 2015

cmsbuild commented Aug 19, 2015

lveldere commented Aug 20, 2015

cmsbuild commented Sep 20, 2015

cmsbuild commented Sep 20, 2015

wmtan commented Sep 22, 2015

slava77 Sep 23, 2015

wmtan Sep 23, 2015

slava77 commented Sep 23, 2015

wmtan commented Sep 24, 2015

ferencek commented Oct 8, 2015

wmtan commented Oct 8, 2015

ferencek commented Oct 8, 2015

wmtan commented Oct 8, 2015

ferencek commented Oct 8, 2015

wmtan commented Oct 8, 2015

ferencek commented Oct 23, 2015

wmtan commented Oct 23, 2015

ferencek commented Oct 23, 2015

wmtan commented Oct 23, 2015

Do not update HepMCProduct in place (producer implementation) #10858

Do not update HepMCProduct in place (producer implementation) #10858

Conversation

wmtan commented Aug 19, 2015

cmsbuild commented Aug 19, 2015

slava77 commented Aug 19, 2015

cmsbuild commented Aug 19, 2015

cmsbuild commented Aug 19, 2015

wmtan commented Aug 19, 2015

cmsbuild commented Aug 19, 2015

lveldere commented Aug 20, 2015

cmsbuild commented Sep 20, 2015

cmsbuild commented Sep 20, 2015

wmtan commented Sep 22, 2015

slava77 Sep 23, 2015

Choose a reason for hiding this comment

wmtan Sep 23, 2015

Choose a reason for hiding this comment

slava77 commented Sep 23, 2015

wmtan commented Sep 24, 2015

ferencek commented Oct 8, 2015

wmtan commented Oct 8, 2015

ferencek commented Oct 8, 2015

wmtan commented Oct 8, 2015

ferencek commented Oct 8, 2015

wmtan commented Oct 8, 2015

ferencek commented Oct 23, 2015

wmtan commented Oct 23, 2015

ferencek commented Oct 23, 2015

wmtan commented Oct 23, 2015