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

EGM GED bug fixes for pre12 (AOD size, cluster double counting, rechit fraction usage) #1986

Merged
merged 36 commits into from Jan 16, 2014

Conversation

lgray
Copy link
Contributor

@lgray lgray commented Jan 10, 2014

Built ontop of pre11.

This PR includes various bugfixes and improvements to AOD size:

  • Fix to properly use rechit fractions for eta/phi width
  • Fix to produce BasicCluster collections for PF-SCs (instead of ref'ing back to particleFlowClusterECAL)
  • Fix photon pixel seed veto not being properly filled.
  • Fix cut on photon SC eT to be consistent across all GED photon producers.
  • Fix instance of cluster double counting that could happen in PFEGammaAlgo.
  • Consistent renaming of SuperCluster accessors.
  • Cut to reduce PF-ECAL SuperCluster collection.
  • Reduced size to interesting rechit collections for PF-ECAL SCs
  • Import PFClusters of PFSCs by edm::Ref instead of by fraction and overlap

bendavid and others added 27 commits December 30, 2013 20:29
…ions (and adapt common code to work for EB/EE/ES)
…ased isolation, fix egammaFastSimValidation_cff.py so that the folder for gedPhotons is produced, add some updated for private plotting scripts
… Et cut for output collections (set to 4GeV currently)
@cmsbuild
Copy link
Contributor

A new Pull Request was created by @lgray (Lindsey Gray) for CMSSW_7_0_X.

EGM GED bug fixes for pre12 (AOD size, cluster double counting, rechit fraction usage)

It involves the following packages:

DQMOffline/EGamma
DataFormats/EgammaCandidates
DataFormats/EgammaReco
DataFormats/ParticleFlowCandidate
DataFormats/PatCandidates
RecoEcal/Configuration
RecoEcal/EgammaClusterAlgos
RecoEcal/EgammaClusterProducers
RecoEcal/EgammaCoreTools
RecoEgamma/Configuration
RecoEgamma/EgammaElectronAlgos
RecoEgamma/EgammaElectronProducers
RecoEgamma/EgammaPhotonAlgos
RecoEgamma/EgammaPhotonProducers
RecoEgamma/EgammaTools
RecoEgamma/Examples
RecoParticleFlow/Configuration
RecoParticleFlow/PFClusterProducer
RecoParticleFlow/PFClusterTools
RecoParticleFlow/PFProducer
RecoTauTag/RecoTau
TauAnalysis/MCEmbeddingTools
Validation/RecoEgamma

@civanch, @thspeer, @danduggan, @mdhildreth, @cmsbuild, @anton-a, @nclopezo, @rovere, @deguio, @slava77, @vadler, @Degano, @ojeda can you please review it and eventually sign? Thanks.
@richard-cms 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.
@ktf you are the release manager for this.

@slava77
Copy link
Contributor

slava77 commented Jan 10, 2014

There is a problem running unit tests (various PAT tests, in fact)

Running script: $CMSSW_BASE/src/PhysicsTools/PatAlgos/test/runtests.sh
globaltag = START70_V4::All
11-Jan-2014 00:22:38 CET  Initiating request to open file root://eoscms//eos/cms/store/relval/CMSSW_7_0_0_pre7/RelValProdTTbar/AODSIM/PRE_ST62_V8-v2/00000/863E36FF-2046-E311-9526-003048678C26.root?svcClass=default
11-Jan-2014 00:22:40 CET  Successfully opened file root://eoscms//eos/cms/store/relval/CMSSW_7_0_0_pre7/RelValProdTTbar/AODSIM/PRE_ST62_V8-v2/00000/863E36FF-2046-E311-9526-003048678C26.root?svcClass=default
Begin processing the 1st record. Run 1, Event 1, LumiSection 1 at 11-Jan-2014 00:22:55.156 CET
Begin processing the 2nd record. Run 1, Event 2, LumiSection 1 at 11-Jan-2014 00:23:32.180 CET
----- Begin Fatal Exception 11-Jan-2014 00:23:32 CET-----------------------
An exception of category 'InvalidReference' occurred while
   [0] Processing run: 1 lumi: 1 event: 2
   [1] Running path 'outpath'
   [2] Calling event method for module PoolOutputModule/'out'
   [3] Calling produce method for unscheduled module PATElectronSelector/'selectedPatElectrons'
   [4] Calling produce method for unscheduled module PATElectronProducer/'patElectrons'
Exception Message:
BadRefCore RefCore: Request to resolve a null or invalid reference to a product of type 'std::vector<reco::SuperCluster>' has been detected.
Please modify the calling code to test validity before dereferencing.
----- End Fatal Exception -------------------------------------------------

@bendavid
Copy link
Contributor

Hi Slava,
These PAT tests take as input some semi-random old relval files from here
https://github.com/cms-sw/cmssw/blob/CMSSW_7_0_X/PhysicsTools/PatAlgos/python/patInputFiles_cff.py

I confirm that producing a new ttbar aod file with this branch and running the test on top of that, things work fine.
(produced with runTheMatrix.py -l 2.0 --useInput=all -e --command="-n 100")

(Does this test even run out of the box in pre11? Because I would have guessed that the change which broke compatibility with the old 700pre7 input files specified here was actually the switch to the new ged sequence as default in pre11)

@cmsbuild
Copy link
Contributor

Pull request #1986 was updated. @civanch, @thspeer, @danduggan, @mdhildreth, @cmsbuild, @anton-a, @nclopezo, @rovere, @deguio, @slava77, @vadler, @Degano, @ojeda can you please check and sign again.

@civanch
Copy link
Contributor

civanch commented Jan 14, 2014

+1

1 similar comment
@vadler
Copy link

vadler commented Jan 14, 2014

+1

@slava77
Copy link
Contributor

slava77 commented Jan 14, 2014

working on it (second iteration)

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Jan 15, 2014

From the last changes (b8e96e0..4100617) , photons are changed the most, as expected, around 10 GeV:
(these are expected)

all_sign293avssign293_singlegammapt10wf18p0c_recophotons_gedphotons__reco_obj_et

particleFlow is quite resilient here
all_sign293avssign293_singlegammapt10wf18p0c_recopfcandidates_particleflow__reco_obj_pt39

@slava77
Copy link
Contributor

slava77 commented Jan 15, 2014

There are more messages of the following kind

%MSG-w PFEGammaAlgo::mergeROsByAnyLink:  PFEGammaProducer:particleFlowEGamma  14-Jan-2014 22:21:51 CET Run: 1 Event: 251
Need to implement proper merging of two gsf candidates!

Based on the increase of the final objects, I'm assuming these are happening more often as expected.
What are the plans to fix this thing?

@lgray
Copy link
Contributor Author

lgray commented Jan 15, 2014

Hi Slava,

Fixing this particular warning requires some fairly careful physics studies in the POGs/PAGs, close-by electrons and early conversions are pretty difficult to disentangle correctly.

If it is too annoying, we can demote it to a LogInfo in another PR to avoid re-signing for a one line change.

@slava77
Copy link
Contributor

slava77 commented Jan 15, 2014

Hi Lindsey,

I'm just taking this as an opportunity to remind that it's there ;)

@lgray
Copy link
Contributor Author

lgray commented Jan 15, 2014

Then it is doing its job! :-)

//in order to be consistent with the cut applied in the
//ElectronSeedProducer
double scetaBeamSpot = (new_sc.position()-beamSpot_->position()).eta();
if ( new_sc.energy()/cosh(scetaBeamSpot) > threshSuperClusterEt_ ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is

Vector deltaR = new_sc.position()-beamSpot_->position();
float et = new_sc.energy()/deltaR.mag()*deltaR.perp();

should be quite a bit lighter than conversion to eta and back

@slava77
Copy link
Contributor

slava77 commented Jan 15, 2014

Just to note, I'm expecting a follow up on the code review in a separate PR, as discussed

@deguio
Copy link
Contributor

deguio commented Jan 16, 2014

+1

@slava77
Copy link
Contributor

slava77 commented Jan 16, 2014

+1

tested 4100617 in CMSSW_7_0_X_2014-01-10-1400 (sign293a test area)

I'll post some more notes later.
Changes in physics as expected.
AOD event content expansion made in pre11 is now recovered

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next IBs unless changes (tests are also fine). @ktf can you please take care of it?

@slava77
Copy link
Contributor

slava77 commented Jan 16, 2014

This PR is for 70X
@davidlange6 , I'm guessing, you will need to +1 the ORP signature

ktf added a commit that referenced this pull request Jan 16, 2014
Reco fix -- EGM GED bug fixes for pre12 (AOD size, cluster double counting, rechit fraction usage)
@ktf ktf merged commit f27501c into cms-sw:CMSSW_7_0_X Jan 16, 2014
lgray added a commit to lgray/cmssw that referenced this pull request Jan 16, 2014
@slava77
Copy link
Contributor

slava77 commented Jan 16, 2014

Here are some more notes for the record:

AOD size, only RECO process in ttbarPU 202.0 (200 events). Numbers are bytes per event

  • gone: recoPFClusters_particleFlowClusterPS__RECO. 54121.7
  • gone: recoPFClusters_particleFlowClusterECAL__RECO. 46983.8
  • gone: recoPhotons_mustachePhotons__RECO. 487.585
  • gone: recoPhotonCores_mustachePhotonCore__RECO. 42.535
  • new: recoCaloClusters_particleFlowSuperClusterECAL_particleFlowBasicClusterECALBarrel_RECO. 2573.01
  • new: recoCaloClusters_particleFlowSuperClusterECAL_particleFlowBasicClusterECALPreshower_RECO. 2259.61
  • new: recoCaloClusters_particleFlowEGamma_EBEEClusters_RECO. 1540.79
  • new: recoCaloClusters_particleFlowSuperClusterECAL_particleFlowBasicClusterECALEndcap_RECO. 1272.76
  • new: recoCaloClusters_particleFlowEGamma_ESClusters_RECO. 1003.67
  • Total went from 448 to 355 kB/evt (added 9kB, removed 102kB)

Timing (look at above 1s/job, or 5ms/evt, 200 events):

  • ~ x2.5 decrease in time of conversionTrackCandidates (30 sec/job now)
  • ~ x2 increase in time for gedPhotons (3sec/job now)

General on physics changes:

  • photon yields increased around 10 GeV (fixed up thresholds)
  • slight increase in passing electrons, in particular in background samples, coincides with MVA becoming more signal-like (this is from the fixed cluster shapes)

Some notable plots
TTbarPU

  • more signal-like MVA (not significant though)
    ttpu38_ele_mva
  • cluster sizes reduced
    ttpu38_ee_sc_number
    ttpu38_eb_bc_size
    ttpu38_eb_sc_number

Ele 35 response slightly decreased (less corrections, since cluster shapes are now changed). The effect for the bulk is pretty tiny.
ele35_poverpgen_vs_eta

@bendavid
Copy link
Contributor

Hi Slava,
A few points FYI.

The decrease in timing for the conversions is due to the fact that the ecal seeding for the conversion tracks is now only running on the reduced collection of clusters from the particleFlowSuperClusterECAL producer rather than the full PFCluster collection.

I'm not sure the x2 increase in gedPhotons is fully expected, but given that the total time is small, this can probably be accounted for by the small effective change in threshold by cutting on the parentSCluster Et rather than the (non-corrected at this stage) refined supercluster Et.

If the foreseen changes and cleanup happen for the refined SuperCluster in 71x, the size of the two collections
new: recoCaloClusters_particleFlowEGamma_EBEEClusters_RECO. 1540.79
new: recoCaloClusters_particleFlowEGamma_ESClusters_RECO. 1003.67

can be expected to become probably an order of magnitude smaller, since they will contain only the relatively small number of CaloClusters added in addition to what is already in the mustache SC. (Whereas for 70x these are simply being duplicated between the two collections as has been discussed)

ktf added a commit that referenced this pull request Jan 17, 2014
Reco fixes -- Fixes to Cluster Shape Variables monitored in EXO DQM + #1986 Code Review
ggovi pushed a commit to ggovi/cmssw that referenced this pull request Jan 11, 2017
Update geant4 to the latest patch.
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

9 participants