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

Protections for jets and MET for PFEGamma and customize function to switch on PFEG in ParticleFlow REDUX #1242

Merged
merged 26 commits into from Nov 1, 2013

Conversation

lgray
Copy link
Contributor

@lgray lgray commented Oct 30, 2013

This is a cherry-picked version of: #1177

built ontop of: CMSSW_7_0_X_2013-10-31-0200 ## note this is in the future! no testing until 10-31-0200 is available

Includes protections for jets and MET when using PFEGamma.
PFEGamma is still turned off by default.

All matrix tests passed in standard settings.

All matrix tests passed using the following customization to switch on PFEG:

runTheMatrix.py -s --command="--customise RecoEgamma/Configuration/customizePFforEGammaGED.customizePFforEGammaGED" --useInput=all

@cmsbuild
Copy link
Contributor

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

Protections for jets and MET for PFEGamma and customize function to switch on PFEG in ParticleFlow REDUX

It involves the following packages:

Validation/RecoEgamma
DataFormats/ParticleFlowCandidate
RecoEgamma/Configuration
DataFormats/CaloRecHit
RecoEcal/EgammaClusterAlgos
HLTriggerOffline/Top
PhysicsTools/PatAlgos
RecoParticleFlow/PFProducer
RecoParticleFlow/Configuration

@smuzaffar, @nclopezo, @danduggan, @rovere, @thspeer, @deguio, @slava77, @vadler, @eliasron can you please review it and eventually sign? Thanks.
@TaiSakuma 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.

@cmsbuild
Copy link
Contributor

Pull request #1242 was updated. @smuzaffar, @nclopezo, @danduggan, @rovere, @thspeer, @deguio, @slava77, @vadler, @eliasron can you please check and sign again.

@cmsbuild
Copy link
Contributor

Pull request #1242 was updated. @smuzaffar, @nclopezo, @danduggan, @rovere, @thspeer, @deguio, @slava77, @vadler, @eliasron can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Oct 31, 2013

@slava77 working on it
... for the moment just twiddling my thumbs, waiting for an IB. Thanks to git for the pedantic merging practices.

@cmsbuild
Copy link
Contributor

Pull request #1242 was updated. @smuzaffar, @nclopezo, @danduggan, @rovere, @thspeer, @deguio, @slava77, @vadler, @eliasron can you please check and sign again.

@cmsbuild
Copy link
Contributor

@lgray
Copy link
Contributor Author

lgray commented Oct 31, 2013

Well, that was unexpected... Cool. Now to wait for 31/10/2013 IB.... Any idea what the hang up is?

@ktf
Copy link
Contributor

ktf commented Oct 31, 2013

The slc6_amd64_gcc481 build is there. You can use it. The slc5_amd64_gcc481
failed due to some configuration problems. I've restarted it right now.

@slava77
Copy link
Contributor

slava77 commented Oct 31, 2013

Lindsey,

Could you please comment on the following (new) log errors/warnings:

  • %MSG-w PFEGammaAlgo::mergeROsByAnyLink: PFEGammaProducer:particleFlowEGamma
    • "Need to implement proper merging of two gsf candidates!"

And errors in the GED-based configuration

  • %MSG-e MissingInput: ReducedRecHitCollectionProducer:reducedEcalRecHitsEB
    • no reason to skip detid from : (pfPhotonInterestingEcalDetIdEB, , )
    • very noisy: twice per event
  • %MSG-e MissingInput: ReducedRecHitCollectionProducer:reducedEcalRecHitsEE
    • as above, very noisy
  • %MSG-e PFEGammaAlgo::unwrapSuperCluster(): PFEGammaProducer:particleFlowEGamma
    • No associated block elements to SuperCluster! This is a bug we should fix!
    • these show up in ~5% of single electron events

@lgray
Copy link
Contributor Author

lgray commented Oct 31, 2013

Hi Slava,

mergeROs and unwrapSuperCluster require some deeper thought. I think, for now, I can provide a patch to not merge two gsf candidates but still issue the warning

The middle two require replacement of some modules that collect used rechits. I'll try to get something soon.

@lgray lgray closed this Oct 31, 2013
@lgray lgray reopened this Oct 31, 2013
@lgray
Copy link
Contributor Author

lgray commented Oct 31, 2013

@slava77 ack, wrong button (hit close accidentally). I hope that doesn't mess with anything. Anyway what I can do for now is remove the two noisy rechit collectors, replacing them with another collector before 700 comes outs. Since it is adapting a feature to the new reco, this should be ok and can come in as a patch to the customize feature.

@cmsbuild
Copy link
Contributor

Pull request #1242 was updated. @nclopezo, @danduggan, @rovere, @thspeer, @deguio, @slava77, @vadler, @eliasron can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Oct 31, 2013

On 10/31/13, 5:27 PM, Lindsey Gray wrote:

@slava77 https://github.com/slava77 ack, wrong button (hit close
accidentally). I hope that doesn't mess with anything. Anyway what I can
do for now is remove the two noisy rechit collectors, replacing them
with another collector before 700 comes outs. Since it is adapting a
feature to the new reco, this should be ok and can come in as a patch to
the customize feature.

Thanks, with the errors silenced, we can have some processing
already.

    --slava


Reply to this email directly or view it on GitHub
#1242 (comment).


Vyacheslav (Slava) Krutelyov
TAMU: Physics Dept Texas A&M MS4242, College Station, TX 77843-4242
CERN: 42-R-027
AIM/Skype: siava16 googleTalk: slava77@gmail.com
(630) 291-5128 Cell (US) +41 76 275 7116 Cell (CERN)


@slava77
Copy link
Contributor

slava77 commented Nov 1, 2013

I'm looking at the diffs now, comparing 1242 with baseline IB in CMSSW_7_0_X_2013-10-31-1400

Diffs in the current (non-GED) mode are affecting only the new collections, as expected.

Not clearly OK things:

  • significant drop in conversions in pfPhotonValidator , by about 20%
    • in a background sample (TTBar PU) from 44 to 31
    • in gamma 35 sample from 1330 to 1160
    • this appears to be limited to the 2-leg conversions
  • pretty large drop in the number of photons with low r9 (these also correspond to low e1x5/e5x5)
    • visible very well in the background sample (TTbar PU)
      screen shot 2013-11-01 at 11 45 19 am
    • also visible in a signal sample (gamma 35, similar picture in electron 35)
      screen shot 2013-11-01 at 11 49 39 am

Good things:

  • the error messages from interesting hits are gone. (in GED mode)
  • photon E/Etrue for high r9 is much better
    screen shot 2013-11-01 at 11 52 44 am

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 1, 2013

@lgray
Copy link
Contributor Author

lgray commented Nov 1, 2013

@slava77 Those changes come from: https://github.com/cms-sw/cmssw/pull/1242/files#diff-bfda53c699a29a8f1e58425363c4485cR1258

The assignment of the seed matters in this case. Sorting by energy yields good agreement with the e/gamma ID variables.

@slava77
Copy link
Contributor

slava77 commented Nov 1, 2013

Hi Lindsey

Thanks for confirming on the low E1x5 and r9 distributions

Just a few more, now in the fwlite script (more details analyzed):

  • ele 35
    all_sign265vsorig_singleelectron35wf17p0c_recogsfelectrons_gedgsfelectrons__reco_obj_e1x5

From the presentation in egamma and yesterday in RECO, this looks like an intended fix.

@lgray
Copy link
Contributor Author

lgray commented Nov 1, 2013

@slava77 This is also due to the seed sorting as seen above.

@slava77
Copy link
Contributor

slava77 commented Nov 1, 2013

+1

Overall, this is a functional PR.
At this point it would be more productive to integrate it now and work on remaining fixes in the experimental areas later, towards the 700 release.
I wish there was one more pre-release cycle to sort out the issues.
We should try to get as many things fixed as possible in the next following days (start on it before RelVals come in)

tested 7e6bbf3 in CMSSW_7_0_X_2013-10-31-1400 as sign 265

Standard mode (no switch over to using the GED objects in PF)

  • The mainstream products are not affected
  • the gedPhotons still have a bug which results a 20% drop in the 2-leg conversions (to be fixed in a separate PR)

GED mode (using customizePFforEGammaGED.py)
More detailed analysis to come.

  • jobs in the matrix workflows are operational
    • no crashes
    • noisy errors from the previous iteration are gone
  • differences are limited to electron and photon objects and what happens downstream
    • minor diffs in PF jets and MET
    • significant increase in tracker-only electrons (expected), there were practically none in the tracker-notEcalDriven category previously
    • significant increase in e+ e- pairs (up to x10 or more depending on a sample) (expected)
    • almost all TeV muons are now electrons (not really expected)
  • some validation/DQM plots are dead empty most likely because they have hardcoded gsfElectron and skip the events when not available
  • MVA variables for electrons are not filled (bug)
  • electron energy response is degraded in the EE-EB crack (eventually to be solved by corrections, but may be a problem of some sort)

@slava77
Copy link
Contributor

slava77 commented Nov 1, 2013

@ktf
Giulio,
with respect to readiness for pre8, Lindsey is going to provide a fix to one problem mentioned above, this will come in a separate "light-weight" PR.

@ktf
Copy link
Contributor

ktf commented Nov 1, 2013

As discussed with @vadler and @slava77 I'll merge this to allow Volker to provide a quick fix he wants for analysis on top of it. Will integrate #1273 once Slava is happy.

@ktf
Copy link
Contributor

ktf commented Nov 1, 2013

@deguio, I'm skipping your signature. Nevertheless shout if not happy.

ktf added a commit that referenced this pull request Nov 1, 2013
Reco updates -- Protections for jets and MET for PFEGamma and customise function to switch on PFEG in ParticleFlow REDUX
@ktf ktf merged commit 0989c19 into cms-sw:CMSSW_7_0_X Nov 1, 2013
@vadler vadler mentioned this pull request Nov 1, 2013
try: label = visitee.label_()
except AttributeError: label = '<Module not in a Process>'
#try: label = visitee.label_()
#except AttributeError: label = '<Module not in a Process>'
Copy link

Choose a reason for hiding this comment

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

This sanity check should not simply be switched off.

@vadler
Copy link

vadler commented Nov 1, 2013

+1
(belated) ... but only in combination with #1287 .
S. https://github.com/cms-sw/cmssw/pull/1242/files#r7372995 .

ktf added a commit that referenced this pull request Nov 5, 2013
Reco updates -- Patches on top of #1242 (Egamma improvements), consistency fixes and conversions
aloeliger added a commit to aloeliger/cmssw that referenced this pull request Apr 9, 2024
Updated seed tower thresholds in endcap to improve efficiency (from cms-sw#44606)
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

6 participants