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

HGCAL recogeometry fix for local (thus also global) x and y coordinates #4911

Merged

Conversation

vandreev11
Copy link
Contributor

@bsunanda, @pfs, @lgray The fix in recogeometry for the correspondence between cell number and the local
xy coordinates. Solves "jumping muon" problem.

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 8, 2014

A new Pull Request was created by @vandreev11 for CMSSW_6_2_X_SLHC.

HGCAL recogeometry fix for local (thus also global) x and y coordinates

It involves the following packages:

Geometry/HGCalCommonData

@civanch, @Dr15Jones, @ianna, @mdhildreth, @cmsbuild, @nclopezo, @Degano, @ktf can you please review it and eventually sign? Thanks.
@ghellwig 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.
@fratnikov, @mark-grimes you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@lgray
Copy link
Contributor

lgray commented Aug 8, 2014

This piece of code appears to also be used in the SIM geometry, is the numbering different there and we're saved from having to re-generate events?

@vandreev11
Copy link
Contributor Author

Well, it comes with the current SIM numbering and matches well with a real muon in reco
after the fix.

@lgray
Copy link
Contributor

lgray commented Aug 8, 2014

Hmm, that is tricky. Can you hand scan 20-30 events using the old SIM to make sure the problem is gone and in the RECO only?

@mark-grimes
Copy link

Simple testing (i.e. only that it doesn't crash workflows) is fine, and the change looks straightforward enough. I'll wait for confirmation from those in this discussion before merging.

@lgray
Copy link
Contributor

lgray commented Aug 12, 2014

@vandreev11 If I understand from our chats in email with Sunanda, this is ready to go in now? or are we waiting for more studies from @bsunanda?

@bsunanda
Copy link
Contributor

I understand this can always go in as a fix

On Tue, 12 Aug 2014, Lindsey Gray wrote:

@vandreev11 If I understand from our chats in email with Sunanda, this is
ready to go in now? or are we waiting for more studies from @bsunanda?


Reply to this email directly or view it onGitHub.[5033146__eyJzY29wZSI6Ik5ld3NpZXM6QmVhY29uIiwiZXhwaXJlcyI6MTcyMzQ1ND
ExNSwiZGF0YSI6eyJpZCI6MzkyMTY5MjZ9fQ==--b4437c4cf89ed4d20bdb98d79c992c8f8ed
1e0c5.gif]

@vandreev11
Copy link
Contributor Author

this fix "effectively" solves the big (10 degrees) hit shifts observed as "jumping muons", though it makes deviation
from the assign-cell-number scheme of the GEN-SIM. The matching of the "non-jumping" muons preserved at a decent level.
I think we can go with this for the time being, until a better solution will be find.
Valeri

On Aug 12, 2014, at 12:17 PM, bsunanda wrote:

I understand this can always go in as a fix

On Tue, 12 Aug 2014, Lindsey Gray wrote:

@vandreev11 If I understand from our chats in email with Sunanda, this is
ready to go in now? or are we waiting for more studies from @bsunanda?


Reply to this email directly or view it onGitHub.[5033146__eyJzY29wZSI6Ik5ld3NpZXM6QmVhY29uIiwiZXhwaXJlcyI6MTcyMzQ1ND
ExNSwiZGF0YSI6eyJpZCI6MzkyMTY5MjZ9fQ==--b4437c4cf89ed4d20bdb98d79c992c8f8ed
1e0c5.gif]


Reply to this email directly or view it on GitHub.

@mark-grimes
Copy link

merge

Okay. There's no production GEN-SIM so no problem from our side, as long as you're happy potentially invalidating your private samples.

cmsbuild added a commit that referenced this pull request Aug 12, 2014
HGCAL recogeometry fix for local (thus also global)  x and y coordinates
@cmsbuild cmsbuild merged commit 906536e into cms-sw:CMSSW_6_2_X_SLHC Aug 12, 2014
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.

5 participants