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

Complete removal of ECAL PF cleaning from reco and HLT, and verify its effect on data #18455

Closed
perrotta opened this issue Apr 25, 2017 · 11 comments

Comments

@perrotta
Copy link
Contributor

With PR #18438 (now merged in 91X) PF cluster cleaning in ECAL clusters got removed from the Reco sequences.

This followed the discussion started in #17859 (and continued in the reco meeting of Apr 21) where it was realized that modifications to the PF spike cleaning originally meant for HF could have affected ECAL PF cleaning as well. In the discussion, it was stated that PF cleaning is not actually needed for ECAL, because spike cleaning is already performed at RecHit level: the decision to remove it from the reco and hlt sequences followed.

ECAL cleaning is also applied at HLT, and therefore another pull request with the removal at HLT is also needed: @Sam-Harper @Martin-Grunewald, @silviodonato : any plan for it? The HF spike cleaning pull request will be integrated as soon as that HLT pull request will also be available.

Furthermore, when looking at the effect of #18438 it was realized (see #18438 (comment)) that it is indeed tiny, but not completely transparent, even for MC samples. I was not able to test on a data sample with actual spikes in ECAL, in order to see which is the effect on them. At some point one should verify that really such a PF cleaning was useless for ECAL spike cleaning (I would say, especially for HLT): @Sam-Harper @rafaellopesdesa could you please verify by running with and without it on some appropriate data sample, and report here the outcome?

Attention: @slava77
Attention: @knash @emanueledimarco @jainshilpi @fwyzard

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 25, 2017

A new Issue was created by @perrotta .

@davidlange6, @Dr15Jones, @smuzaffar can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

@Sam-Harper
Copy link
Contributor

I'll file a JIRA today for the HLT part. As for testing for data, I have no time to do this. Again, I consider the existence of this bug and I'm frankly annoyed that PF guys felt that they had to reinvent the wheel. I agree there is a possibility that it is hiding and cancelling out another bug.

I hate to buck pass, but this is a really a PF problem (its in the generic PFClusters) and PF should test and deal with it. They should justify why they were re-implimenting spike cleaning instead of using the ECAL flags.

@Sam-Harper
Copy link
Contributor

Btw when I remove this, I'm removing this for all paths. It'll also effect JetMET as well. Just so people are aware this is a global problem with PFClustering that effects everything.

@Dr15Jones
Copy link
Contributor

assign reconstruction

@cmsbuild
Copy link
Contributor

New categories assigned: reconstruction

@slava77,@perrotta you have been requested to review this Pull request/Issue and eventually sign? Thanks

@rafaellopesdesa
Copy link
Contributor

@perrotta Thanks. I will try to check the effect of this change in EGM in the coming days (probably not in the next 2 or 3, but after that I should have time... I apologize for not being able to do immediately).

@perrotta
Copy link
Contributor Author

Thank you @rafaellopesdesa : the time schedule you propose is fine. I just would like to have it documented, and we can wait a few days for it.

@Sam-Harper
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Jun 28, 2017

@perrotta
I suppose we can "+1" and close this issue now

@perrotta
Copy link
Contributor Author

+1
JIRA ticket CMSHLT-1242 integrated in the HLT menu since /dev/CMSSW_9_1_0/HLT/V12

@cmsbuild
Copy link
Contributor

This issue is fully signed and ready to be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants