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

enable Meantimer reco in cosmic sequence - backport to 73X #7665

Merged

Conversation

ptraczyk
Copy link
Contributor

Backport of #7177 and #7044 to 73X - enabling MT pattern reco in the cosmic sequence and a small bugfix and extension of the algorithm itself.

@cmsbuild
Copy link
Contributor

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

enable Meantimer reco in cosmic sequence - backport to 73X

It involves the following packages:

RecoLocalMuon/Configuration
RecoLocalMuon/DTSegment

@cmsbuild, @cvuosalo, @nclopezo, @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.
@nclopezo you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@vlimant
Copy link
Contributor

vlimant commented Feb 11, 2015

Note that this consists of an event content change. please bare in mind while bringing this up to deployement at Tier0.

@ptraczyk
Copy link
Contributor Author

yes, true, there was also some cleanup done here and we removed an alternative DT segment collection that was unused for a long time (the "NoDrift" algorithm). The producer for these is also removed.

@slava77
Copy link
Contributor

slava77 commented Feb 11, 2015

From my logs of testing these in 74X, it appears that there is no actual change in the output products.
The changes in the eventContents file was just a cleanup of unused lines.

Piotr, could you please confirm now (before I get to testing this)

@slava77
Copy link
Contributor

slava77 commented Feb 11, 2015

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@ptraczyk
Copy link
Contributor Author

yes, it looks like the NoDrift algo is not being called in the standard
sequence, its defined in MuonLocalRecoGR
https://github.com/cms-sw/cmssw/pull/7665/files?diff=split#diff-1743670c5216aec42bf78885b5b10d49L61

but ReconstructionCosmics doesn't call that but instead calls the
sub-component paths directly:
https://github.com/ptraczyk/cmssw/blob/DTmeantimerCosmicsBackportTo73X/Configuration/StandardSequences/python/ReconstructionCosmics_cff.py#L47

so indeed it looks like in standard cosmic reco the NoDrift segments that
are here removed from the event content are anyway never produced

On Wed, Feb 11, 2015 at 2:17 PM, cmsbuild notifications@github.com wrote:

The tests are being triggered in jenkins.


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

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Feb 11, 2015

... I'm checking it

@slava77
Copy link
Contributor

slava77 commented Feb 11, 2015

+1

for #7665 dff727e

the code changes are the same as in the combination of #7044 and #7177

I confirmed in RECO steps of the short matrix that the lists of stored products are unchanged
(sizes vary for several branches, but that's expected)

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_3_X IBs unless changes (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @nclopezo, @ktf, @smuzaffar

@davidlange6
Copy link
Contributor

+1

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

5 participants