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

Replace CLHEP matrices by root::SMatrix in segment fitter #5125

Merged
merged 3 commits into from Sep 8, 2014

Conversation

ptcox
Copy link
Contributor

@ptcox ptcox commented Sep 1, 2014

This is the straight swap of CLHEP matrices by root::SMatrix in the least-squares fitting of CSC segments. Really the code needs a lot of factoring and general clean-up but I don't have the time - a good place to start would be to factor out the segment-fitting. (That would probably also help GEM since I think they also make use of the same code, by cut-and-paste from CSC.) I also fixed what I consider a bug where after manipulations apparently to improve the numerical robustness of covariance matrix handling, the symmetricity of the covariance matrix was not maintained. This leads to ~8% of segments receiving slightly difference local positions and directions.I decided not to change the ME1/1A max rechits in cluster from 24 to 20, as in the other chambers, even though in postls1 the channels are now unganged - it seems that would be more effort than it's worth (would required setting up separate pre- & post-ls1 configs, AND changing HLT configs to match.) Finally, I noticed there is something called a 'yweightPenaltyThreshold' that was not set for ME4/2. I used the same value as set for ME/2 and ME3/2. I made a lot of tests comparing the original code, the new code, the new code with general rather than explicitly symmetric SMatrix, and the old code with the bug-fix enforcing symmetric covariance matrices. Everything seems consistent. First results of modified code presented in CSC Meeting of 20-Aug-2014, https://indico.cern.ch/event/336061/. Since then I have also increased the study samples to 1000 events of run 1 SingleMu, giving 4500 segments, and everything still makes sense. The runTheMatrix reco test ran fine.

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 1, 2014

A new Pull Request was created by @ptcox for CMSSW_7_2_X.

Replace CLHEP matrices by root::SMatrix in segment fitter

It involves the following packages:

RecoLocalMuon/CSCSegment

@cmsbuild, @nclopezo, @StoyanStoynev, @slava77 can you please review it and eventually sign? Thanks.
@bellan 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.

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 1, 2014

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 2, 2014

@StoyanStoynev
Copy link
Contributor

Tested 2061bbf on top of CMSSW_7_2_X_2014-09-05-1400.
The observed differences are expected due to the bug fix. CSC segments are affected a bit (here I am showing wf 22, 1 TeV muons; lower pt muons or non-muon wf are affected much less):
all_extended_pull5125__vs__baserel_singlemupt1000wf22p0c_cscdetidcscsegmentsownedrangemap_cscsegments__reco_obj_collection__data__localposition_x
all_extended_pull5125__vs__baserel_singlemupt1000wf22p0c_cscdetidcscsegmentsownedrangemap_cscsegments__reco_obj_collection__data__localposition_y
all_extended_pull5125__vs__baserel_singlemupt1000wf22p0c_cscdetidcscsegmentsownedrangemap_cscsegments__reco_obj_collection__data__localpositionerror_xy

The affect the STA and global fits:
all_extended_pull5125__vs__baserel_singlemupt1000wf22p0c_globalmuontracks_eta
all_extended_pull5125__vs__baserel_singlemupt1000wf22p0c_log10absrecotracks_tevmuons_dyt_reco_obj_pz
and also MET:
all_extended_pull5125__vs__baserel_singlemupt1000wf22p0c_log10recopfmets_pfmet__reco_obj_pt

Again, in other wf ( ttbar events), changes - in particular MET - are invisible.

@StoyanStoynev
Copy link
Contributor

+1
Based on the review above, no issues with the code itself.
The PR gets rid of CLHEP matrices in the local CSC code and fixes a long lived bug. Changes affect mostly higher pt muons, PF objects are overall hardly affected, especially in typical running. In any case analyses should benefit from the bug fix.

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 8, 2014

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_2_X IBs unless changes (tests are also fine).

nclopezo added a commit that referenced this pull request Sep 8, 2014
Replace CLHEP matrices by root::SMatrix in segment fitter
@nclopezo nclopezo merged commit 5a19e14 into cms-sw:CMSSW_7_2_X Sep 8, 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.

None yet

4 participants