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

Backport of ecal pfcluster corrections to 73x #7690

Closed
wants to merge 10 commits into from

Conversation

bendavid
Copy link
Contributor

May still change in case problems are seen in 74x jet met validation.

bendavid and others added 10 commits February 4, 2015 18:12
…ulated or stored for now to avoid changing calocluster dataformat), conditions from esprefer for now
…o propagates to any derived CaloClusters associated with the GED electron and photon objects)
…ns are default in fillDescriptions (so HLT keeps using it for now) but MVA corrections are enabled in python for default particle flow sequence in RECO. Also small optimization for energy reciprocal
@cmsbuild
Copy link
Contributor

A new Pull Request was created by @bendavid (Josh Bendavid) for CMSSW_7_3_X.

Backport of ecal pfcluster corrections to 73x

It involves the following packages:

DataFormats/CaloRecHit
RecoEcal/EgammaCoreTools
RecoHI/Configuration
RecoParticleFlow/PFClusterProducer
RecoParticleFlow/PFProducer

@cmsbuild, @cvuosalo, @nclopezo, @slava77 can you please review it and eventually sign? Thanks.
@echapon, @mmarionncern, @Sam-Harper, @MiheeJo, @jazzitup, @argiro, @appeltel, @richard-cms, @yenjie, @kurtejung, @lgray, @mandrenguyen, @dgulhan, @bachtis, @yetkinyilmaz 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.
@nclopezo you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@mandrenguyen
Copy link
Contributor

@bendavid
I must have missed it in the 74X PR, but the following line looks suspicious to me:
particleFlowClusterECAL.energyCorrector.verticesLabel = cms.InputTag('hiPixelAdaptiveVertex')
hiSelectedVertex is the pixel-track vertex collection used for essentially everything in the HI reco.
hiPixelAdaptiveVertex is an intermediate step.
Please switch to hiSelectedVertex, unless there's a good justification for this.

@bendavid
Copy link
Contributor Author

Ok, but I think we can fix it later (and first in 74x)

The vertex collection is ONLY used to count the number of vertices, and there's an inconsistency there with the pp training no matter what. So I would say the difference is not relevant.

@mandrenguyen
Copy link
Contributor

Thanks for the info. I still suggest to fix this at some point to avoid confusion.
Apparently someone is going to have run the training procedure for the HI sequence, if this is essential for particle flow.

@bendavid
Copy link
Contributor Author

The pp trained corrections may be only mildly suboptimal, but will need to be checked. (The old corrections are still available with a configuration switch at least)

@slava77
Copy link
Contributor

slava77 commented Feb 12, 2015

Josh,
is the number of mostly a proxy to the OOT PU or is its main target the in-time PU?
The OOT PU virtually doesn't exist due to > 200 ns bunch spacing.
If the corrections depend strongly on ambient occupancy, then for HI it would be more appropriate to use something like track multiplicity (this can vary from a few hundred in minbias-like events to close to over a few K (3-5K) depending on the kind of collision).

Matt,
what's the expected pileup in the upcoming HI runs? What was the maximum in the Run-1 data?

@mandrenguyen
Copy link
Contributor

Slava, the probability of two PbPb collisions in the same bx, was on the order of 1% in 2011. For 2015 it will be on the order of several times that. OOT will still be negligible.

@bendavid
Copy link
Contributor Author

The number is targeting mainly the in time pileup.

Agreed that track multiplicity or rho might be a better proxy in that case.
(Matteo is also looking at related questions for the HLT)

@bendavid
Copy link
Contributor Author

To first order the consqeuence of getting this wrong is "just" a few % scale shift.

@slava77
Copy link
Contributor

slava77 commented Feb 12, 2015

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Feb 13, 2015

Hi Josh,

was there a request to update 73X GT to include the ecal pf calibration payloads?
ESPrefer is a stopgap.

What's the purpose of this backport, BTW?
I'm guessing to finalize the run2 MC high level object calibration; is this correct?

@bendavid
Copy link
Contributor Author

Well eventually it's needed for that, but we were at some point going to wait until the jet and met performance had been checked in 74x. (From relvals I suppose). Since if there are problems then the corrections might still need to be updated.

Since the JetMet group can't or won't do any private checks in 74x, they insisted to have the 73x branch sooner, so I made it. Then I got annoyed and just made the pull request. (As I said on the mail thread in sw-devel, I make the pull request in case it's needed.

It can also just sit here and be either merged or updated when the final ok of the corrections is ascertained in 74x. Someone should figure out what is the fastest/most convenient way to get higher stats checks on the jet/met performance. (Jet energy scale and resolution vs eta would go a LONG way)

@bendavid
Copy link
Contributor Author

As for whether the corrections are in 73x global tags, I don't know if @dguida added them also there or not.

@slava77
Copy link
Contributor

slava77 commented Feb 13, 2015

@diguida
Salvatore, can you pick these up in 73X as well?

@diguida
Copy link
Contributor

diguida commented Feb 13, 2015

@slava77 @bendavid queueing them...

@diguida
Copy link
Contributor

diguida commented Feb 25, 2015

@slava77 @bendavid
I was finally able to have the tests running fine also in 73X.
See #7941 with the changes in the labels within the consumer code and the new GTs.

@slava77
Copy link
Contributor

slava77 commented Feb 26, 2015

-1

superseded by #7941

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