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

Cosmics dt reco fix #7044

Merged
merged 3 commits into from Jan 9, 2015
Merged

Cosmics dt reco fix #7044

merged 3 commits into from Jan 9, 2015

Conversation

ptraczyk
Copy link
Contributor

@ptraczyk ptraczyk commented Jan 5, 2015

Meantimer reconstruction is enabled in the cosmic reco sequences. A small tweak was also done in the MT algo - previously the algorithm interpreted segments with time within 20ns of nominal as "in time" and fit segment position and direction assuming time offset ==0. Now this is configurable and switched off for cosmics - for cosmics the 3 parameter fit with time floating is performed always.

Validation results:
http://cmsdoc.cern.ch/~ptraczyk/cosmicMuons.pdf
http://cmsdoc.cern.ch/~ptraczyk/cosmicMuons1Leg.pdf
http://cmsdoc.cern.ch/~ptraczyk/globalCosmicMuons.pdf
http://cmsdoc.cern.ch/~ptraczyk/globalCosmicMuons1Leg.pdf
(efficiency and resolution improve)

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 5, 2015

A new Pull Request was created by @ptraczyk (Piotr Traczyk) for CMSSW_7_4_X.

Cosmics dt reco fix

It involves the following packages:

RecoLocalMuon/Configuration
RecoLocalMuon/DTSegment

@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.
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.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 6, 2015

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 6, 2015

@@ -7,16 +7,17 @@
# The linear DriftFromDB algos is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

technically, these are not module initializer files
but none of it has been enforced by the config file model

https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideAboutPythonConfigFile#Including_Standard_Module_defini

Copy link
Contributor

Choose a reason for hiding this comment

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

meaning, _cff is a more appropriate suffix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this would apply to nearly all of the config files here, right? We could clean this up at some point, though this would require some care as these files are being then referenced in a few other places ..

Copy link
Contributor

Choose a reason for hiding this comment

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

right.
It's something to put on the list.
Doesn't have to come with this PR.

@slava77
Copy link
Contributor

slava77 commented Jan 8, 2015

Hi Piotr

Was this presented in the muon POG meeting already?

@@ -122,16 +125,16 @@ void DTSegmentUpdator::fit(DTRecSegment4D* seg, bool allow3par) const {
if(seg->hasZed()) {

// fit in-time Phi segments with the 2par fit and out-of-time segments with the 3par fit
if (fabs(seg->phiSegment()->t0())<40.) {
fit(seg->phiSegment(),allow3par,0);
Copy link
Contributor

Choose a reason for hiding this comment

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

this part "allow3par, block3par" had block3par change from 1 in #3724 (~original version) to 0 in #4018, to now back to 1 here.
Is this just empirical (possibly riding a random variation wave) or is there reason behind each time this being an improvement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous change was an error. I spotted this and reverted to the original version. "block3par" blocks the 3 parameter fit, which is supposed to happen if the segment is "in time". then in line 382 the 2-par fit is executed and it replaces the result of the 3-par fit.
The cutoff it now configurable via "intime_cut" so if we set it to -1 "block3par" has no effect - this is what is done in the cosmic sequences now.

@ptraczyk
Copy link
Contributor Author

ptraczyk commented Jan 8, 2015

The intention was to have intime_cut = -1 uncommented from the very
beginning - that was the reason for the parameter to be added in the first
place, before this cut was hardcoded. I believe all the tests that I linked
earlier were done with it uncommented and the fact that it was not present
in the original PR was just an error on my side, made when I was preparing
the code for the pull request (my previous working branch was based on 72).

Right now I'm trying to double-check this (the comparison plots)

On Thu, Jan 8, 2015 at 2:50 PM, Slava Krutelyov notifications@github.com
wrote:

Hi Piotr,

I'll see what comes out from our RECO comparisons (locally run)
and if the diff is confirmed to be small we can proceed.

Please comment on now uncommented intime_cut = -1 in cosmics,
which came with the last commit.

On 1/8/15 7:34 AM, Piotr Traczyk wrote:

hi Slava,
the block3par change has very little effect, I can try to make some
plots to quentify it, but anyhow the befavior before this PR is
logically incorrect - out-of-time segments are fit with 2-par fit and
in-time segments with 3-par fit.
For collision muons the difference is very slight, for things like
cosmics its more visible - if the muons are in time, allowing time to
float (3-par fit) or fixing it to zero (2-par fit) is almost the same
(the latter should have a tiny advantage for example it a segment is
in-time, but the fit thinks it's off by a few ns).


Reply to this email directly or view it on GitHub
#7044 (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)



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

@slava77
Copy link
Contributor

slava77 commented Jan 8, 2015

Are there plans to update the cosmic data reconstruction (cosmic scenario in the matrix)?

@slava77
Copy link
Contributor

slava77 commented Jan 9, 2015

+1

for #7044 e159def

based on tests in CMSSW_7_4_X_2015-01-07-1000 (test area sign485)

Some observations

  • cosmic reco in cosmic scenario is unchanged
  • most changes (in pp configuration), as expected start from dt segments and then propagate downstream. changes in the cosmic muon sequences are larger.
  • the "regular" muon reco is visibly affected only in chi2 of the DT segments, the effects at higher level are much smaller

1TeV muon sample
all_sign485vsorig_singlemupt1000wf22p0c_mindtchamberiddtrecsegment4dsownedrangemap_dt4dsegments__reco_obj_collection__data__chi2 99_99
all_sign485vsorig_singlemupt1000wf22p0c_mindtchamberiddtrecsegment4dsownedrangemap_dt4dcosmicsegments__reco_obj_collection__data__chi2 99_99

10 GeV muon gun
all_sign485vsorig_singlemupt10wf20p0c_mindtchamberiddtrecsegment4dsownedrangemap_dt4dcosmicsegments__reco_obj_collection__data__chi2 99_99
all_sign485vsorig_singlemupt10wf20p0c_mindtchamberiddtrecsegment4dsownedrangemap_dt4dsegments__reco_obj_collection__data__chi2 99_99

time distribution for cosmics reco change quite a bit (as I understand, as expected)
all_sign485vsorig_singlemupt10wf20p0c_recomuons_muonsfromcosmics__reco_obj_time_timeatipinout

in ttbar with PU35@25ns (wf 25202) the regionalCosmicCkfTrackCandidates has some fluctuations affecting mostly CPU cost: this module takes about 5 sec to run on events with muons (almost as slow as all tracking ... not related to this PR, just mostly a reminder that some review and fixing is pending there

muon pt10 also shows visibly better residuals
wf20_4d_res_rzsl
wf20_thetaresw2

Again, for pt=10 GeV muons: There seems to be more hits on the DYT muon fits (good, but not quite obvious)
wf20_dyt_nhits

@slava77
Copy link
Contributor

slava77 commented Jan 9, 2015

please test
(to get jenkins auto-merge)

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 9, 2015

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 9, 2015

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_4_X IBs unless changes or unless it breaks tests.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 9, 2015

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 9, 2015

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

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