Valuemaps for E/gamma GED Integration #1032
Conversation
|
A new Pull Request was created by @lgray (Lindsey Gray) for CMSSW_7_0_X. Valuemaps for E/gamma GED Integration It involves the following packages: DataFormats/EgammaCandidates @nclopezo, @smuzaffar, @thspeer, @slava77 can you please review it and eventually sign? Thanks. |
| // Read the collection of PFCandidates | ||
| edm::Handle<reco::PFCandidateCollection> pfCandidates; | ||
|
|
||
| bool found = event.getByLabel(egmPFCandidateCollection_, pfCandidates); |
There was a problem hiding this comment.
I think that all new code should come in with getByToken
There was a problem hiding this comment.
Lindsey,
Since getByToken will require changes in the base class, that may deserve a separate update in this case.
What's your schedule to complete the "consumes" migration?
Slava
There was a problem hiding this comment.
For some of these classes I'll need to call consumes in the parent producer and use getByLabel in the class that does the work since the daughter class doesn't inherit from EDConsumerBase.
At a later time we should do it correctly with the tokens, otherwise there needs to be some rather substantial work done to rewrite some classes.
There was a problem hiding this comment.
Than being said, I think those classes that inherit from producer pose no problem for the migration.
There was a problem hiding this comment.
Ok, I found a reasonably non-invasive way to provide the Tokens down to the owned classes.
I'll do this.
There was a problem hiding this comment.
Why not provide a consumesCollector to the helper, the class that does the work, as explained on
https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideEDMGetDataFromEvent#Consumes_and_Helpers ?
There was a problem hiding this comment.
Neat trick, thanks, I'll keep it around for the future. For now, everything is changed to either use tokens directly or use consumes + getByLabel (only one case of this).
|
@slava77 working on it |
| @@ -1,3 +1,7 @@ | |||
| <use name="FWCore/Framework"/> | |||
There was a problem hiding this comment.
these are duplicates, please cleanup
Here are scram warnings
**WARNING: Multiple usage of "FWCore/Framework". Please cleanup "use" in "non-export" section of "src/RecoEgamma/Examples/BuildFile".
**WARNING: Multiple usage of "FWCore/ParameterSet". Please cleanup "use" in "non-export" section of "src/RecoEgamma/Examples/BuildFile".
|
Hi Lindsey, I see quite a few differences, in an apparent contradiction to "Don't expect much if any change in the physics output." Here are a couple of plots from the electron pt=35 sample. Is this an expected outcome of a new implementation/configuration of the gedGsfElectrons ? |
|
Hi Slava, it is normal. It corresponds to the deactivation of the "old" corrections which are not adequate for ged electrons. |
|
Thanks Florian! I had forgotten that. |
|
Lindsey, |
|
Slava, I am curious, can I know which tool you have used to spot the two plots which have changed with the new code ? |
|
@beaudett |
|
@slava77 consumes migration passes short matrix. |
|
Pull request #1032 was updated. @perrotta, @smuzaffar, @nclopezo, @fwyzard, @Martin-Grunewald, @thspeer, @slava77 can you please check and sign again. |
|
@nancymarinelli @beaudett @lgray Is this pull request complete now, or should we expect some more last minute changes? Lindsey confirmed that the "consumes migration" is done here Please clarify Thanks Slava |
|
My two cents: for the producers running inside the HLT, I would strongly For reco-only producers I don't mind ;-) Ciao, On 11 October 2013 11:20, slava77 notifications@github.com wrote:
|
|
+1 |
|
Pull request #1032 was updated. @perrotta, @smuzaffar, @nclopezo, @fwyzard, @Martin-Grunewald, @thspeer, @slava77 can you please check and sign again. |
|
@nancymarinelli @beaudett @slava77 |
|
Just FYI: I started testing preemptively. |
|
Hi it is ok for me. F. |
|
Ok for me as well. Nancy Marinelli Research Associate Professor CERN, Bdg 40/3-A01, 1211 Geneva, SWITZERLAND Phone +41-22-76-70809, fax +41-22-76-78940 From: Lindsey Gray <notifications@github.commailto:notifications@github.com> @nancymarinellihttps://github.com/nancymarinelli @beaudetthttps://github.com/beaudett @slava77https://github.com/slava77 — |
|
Hi Lindsey, This doesn't merge into 70X anymore. CONFLICT (content): Merge conflict in RecoEgamma/EgammaElectronAlgos/src/SiStripElectronSeedGenerator.cc I suspect this is because of interference with #1063 which was just added to 70X Please take a look Thanks Slava |
|
@slava77 hopefully that doesn't clobber anything else... This was made on top of CMSSW_7_0_X_2013-10-13-0200 |
|
Pull request #1032 was updated. @deguio, @ianna, @Martin-Grunewald, @mommsen, @thspeer, @rcastello, @perrotta, @civanch, @vlimant, @fabiocos, @fwyzard, @eliasron, @ktf, @vciulli, @smuzaffar, @Dr15Jones, @demattia, @rovere, @mdhildreth, @giamman, @slava77, @ggovi, @vadler, @mulhearn, @apfeiffer1, @nclopezo, @danduggan, @emeschi, @franzoni can you please check and sign again. |
|
Hi Lindsey Diff stats don't agree with you. |
|
I think this needs to be rebased onto 'CMSSW_7_0_X' branch |
|
Ok, tried that at first, gave some odd output. I'll rewind and try again. |
…non-existant products
|
@slava77 rebased, passes short matrix |
|
Pull request #1032 was updated. @perrotta, @smuzaffar, @nclopezo, @fwyzard, @Martin-Grunewald, @thspeer, @slava77 can you please check and sign again. |
|
+1 |
|
+1 tested 29539e1 in CMSSW_7_0_X_2013-10-13-0200 There are no new differences in physics outputs from the consumes migration. On the timing side there is some odd vacillation in ecalDrivenElectronSeeds (using total time in the module in wflow 202.0, TTBar+PU, 200 events ):
Considering that the final physics output doesn't change, I just leave this noted; maybe someone bothers to check. |
|
This pull request is fully signed and it will be integrated in one of the next IBs unless changes or unless it breaks tests. @ktf can you please take care of it? |
|
@nclopezo can you look at http://cmssw.cvs.cern.ch/cgi-bin/cmssw.cgi/UserCode/slava77/RecoTools/validate.C?view=log and add it to the jenkins stuff? |
Reco improvement -- Valuemaps for E/gamma GED Integration
|
Hi Slava, you run validate.C using the file validateJR.sh? http://cmssw.cvs.cern.ch/cgi-bin/cmssw.cgi/UserCode/slava77/RecoTools/validateJR.sh?view=log Could you give me an example of the command that you run? Thanks |
|
Hi David, bd=commonPartOfDirecotryName the matrix*.txt file is a map for the input files |
…_2_12_4 Add caloParamsHI_2022_v0 2 for heavy ion studies
Machinery to produce PFEG <-> GEDPhoton/GsfElement valuemaps needed for the final step of particle flow reconstruction.
Electron energy/resolution will change since we have dropped the old electron energy corrections for the new GEDGsfElectrons.
Preliminary tests on ttbar indicate the electron reconstruction takes a rather large chunk of time, will investigate further with igprof.
The photon reco doesn't add much.
@beaudett Would like to run a few more tests on his end and possibly change some configurations, but it would be useful to go ahead and start the process since the pieces are in place.
Short matrix results:
7 6 5 5 3 tests passed, 0 1 0 0 0 failed
The one failure is from an empty DBS query result for 8.0.