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

Updates in DT segments reco; bugfix in DT timing in MuonIdentification #1690

Closed
wants to merge 28 commits into from

Conversation

namapane
Copy link
Contributor

@namapane namapane commented Dec 5, 2013

-Bugfix in DT timing in RecoMuon/MuonIdentification

-Rearranged DT segment building code + updates to DT MT pattern reco.
(regression-tested: http://cmsdoc.cern.ch/~ptraczyk/plots_700.pdf)

-Validation/DTRecHits: moved histogram booking to BeginRun, as requested (#1004 (comment))

  • updates to validation histograms and tools for plotting.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 5, 2013

A new Pull Request was created by @namapane (Nicola Amapane) for CMSSW_7_0_X.

Updates in DT segments reco; bugfix in DT timing in MuonIdentification

It involves the following packages:

RecoMuon/TrackingTools
Validation/DTRecHits
RecoLocalMuon/DTSegment
RecoMuon/MuonIdentification

@nclopezo, @danduggan, @rovere, @cmsbuild, @thspeer, @deguio, @slava77, @eliasron can you please review it and eventually sign? Thanks.
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.
@ktf you are the release manager for this.

@slava77
Copy link
Contributor

slava77 commented Dec 12, 2013

@slava77 working on it

@@ -0,0 +1,261 @@
//------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this file extension? .r?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a root script. It's just used for some manual plotting.

Nicola

On 12-Dec-13 11:55, slava77 wrote:

In Validation/DTRecHits/test/writeSummaryTable.r:

@@ -0,0 +1,261 @@
+//------------------------------

Why this file extension? .r?


Reply to this email directly or view it on GitHub
https://github.com/cms-sw/cmssw/pull/1690/files#r8296405.

@slava77
Copy link
Contributor

slava77 commented Dec 12, 2013

@ktf @nclopezo
Giulio and David,
Do I need to send a separate message to get basic jenkins tests?
This PR is a few days old.

@slava77
Copy link
Contributor

slava77 commented Dec 12, 2013

-1

I tried in CMSSW_7_0_X_2013-12-05-0200 (sign279)

An exception of category 'Configuration' occurred while
[0] Constructing the EventProcessor
[1] Constructing module: class=DTRecSegment4DProducer label='dt4DCosmicSegments'
Exception Message:
MissingParameter: Parameter 'enable_3par_fit' not found.

@namapane
Nicola, Please take a look.
It looks like some commits may be missing.

@ktf
Copy link
Contributor

ktf commented Dec 12, 2013

@slava77 the tests are started by hand. @nclopezo did you overlook this one or there is some specific problem?

@nclopezo
Copy link
Contributor

Hi,

I just queued the tests, I must have overlooked this one, sorry.

@slava77
Copy link
Contributor

slava77 commented Dec 12, 2013

just to remind the instructions,
https://twiki.cern.ch/twiki/bin/viewauth/CMS/RecoIntegration#Runtime_Check_FS_and_HLT

runTheMatrix.py -s --useInput all
should complete before we can proceed

Cheers

Slava

if ((chi2/(assHits.size()-3)<theMaxChi2)) return seg;
else {
delete seg;
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

return nullptr;

@namapane
Copy link
Contributor Author

Let me add @ptraczyk to the thread, since he is the developer.

Some comments:

  • The error reported by @nclopezo is the same already reported by @slava77 : one new parameter needs to be added to the cfg of the cosmic reco as well.
  • @deguio: this std::bad_alloc looks a bit strange, can you provide more detaisl on how to reproduce this?
  • @ptraczyk, can you implement the changes suggested by @Dr15Jones ?

Thanks
Nicola


// If no more iterations - store the current segment
DTSegmentCand* seg = fitWithT0(sl,assHits, chi2l, t0_corrl,debug);
if (!seg->good()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will lead to a seg-fault if seg==0 which is one of the values the call can return.

@cmsbuild
Copy link
Contributor

Pull request #1690 was updated. @nclopezo, @danduggan, @rovere, @cmsbuild, @anton-a, @thspeer, @deguio, @slava77, @eliasron can you please check and sign again.

@deguio
Copy link
Contributor

deguio commented Dec 17, 2013

ciao @namapane
I am testing the pull request, but I get a segFault.

I guess that the interesting frame of the bt is:
#5 0x00002b46b9dd1f5c in MuonSegmentMatcher::matchDT(reco::Track const&, edm::Event const&) () from /build/deguio/releaseIntegration/70X/17122013/CMSSW_7_0_X_2013-12-16-0200_1690/lib/slc5_amd64_gcc481/libRecoMuonTrackingTools.so

if you want to have a look at the full bt, you can login on vocms123:/build/deguio/releaseIntegration/70X/17122013/CMSSW_7_0_X_2013-12-16-0200_1690/src/DQMServices/Components/test

@slava77
Copy link
Contributor

slava77 commented Dec 17, 2013

-1

Tested in CMSSW_7_0_X_2013-12-16-0200
All HLT steps failed
[1] Constructing module: class=DTRecSegment4DProducer label='hltDt4DSegments'
Exception Message:
MissingParameter: Parameter 'enable_3par_fit' not found.

The wflows without HLT failed in reco step

%MSG-e FatalSystemSignal: MuonIdProducer:muonsFromCosmics 17-Dec-2013 19:40:22 CET Run: 177790 Event: 280479856
A fatal system signal has occurred: segmentation violation
%MSG

I guess I should've waited for a clear message that this is now good to go.

@namapane
Copy link
Contributor Author

@ptraczyk may want to comment, but I think the commit should include the changes suggested by @Dr15Jones.

Before testing it further we also need the new parameter to be included in the HLT config in confDB.
(The notification of the update of the branch is automatic, I think, it was not a request to retest it right away)

Piotr, can you check the segfault in cosmic reco in the path without HLT in the meanwhile?

Cheers,
Nicola

@slava77
Copy link
Contributor

slava77 commented Dec 17, 2013

On 12/17/13, 9:01 PM, Nicola Amapane wrote:

@ptraczyk https://github.com/ptraczyk may want to comment, but I think
the commit should include the changes suggested by @Dr15Jones
https://github.com/Dr15Jones.

Before testing it further we also need the new parameter to be included
in the HLT config in confDB.
(The notification of the update of the branch is automatic, I think, it
was not a request to retest it right away)

Piotr, can you check the segfault in cosmic reco in the path without HLT
in the meanwhile?

OK, no problems.
I'll wait for a clear "go ahead, it's ready" next time.

Cheers

    --slava

Cheers,
Nicola


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


Vyacheslav (Slava) Krutelyov
TAMU: Physics Dept Texas A&M MS4242, College Station, TX 77843-4242
CERN: 42-R-027
AIM/Skype: siava16 googleTalk: [email protected]
(630) 291-5128 Cell (US) +41 76 275 7116 Cell (CERN)


@deguio
Copy link
Contributor

deguio commented Dec 17, 2013

-1

@ptraczyk
Copy link
Contributor

yes, sorry for the confusion, this wasn't the wersion that's supposed to be
working yet - as @namapane said I added in the changes from @Dr15Jones, but
the HLT is not fixed yet. I will investigate the segfault in cosmics, this
shoudln't be happening.

best,
Piotr

2013/12/17 slava77 [email protected]

On 12/17/13, 9:01 PM, Nicola Amapane wrote:

@ptraczyk https://github.com/ptraczyk may want to comment, but I think

the commit should include the changes suggested by @Dr15Jones
https://github.com/Dr15Jones.

Before testing it further we also need the new parameter to be included
in the HLT config in confDB.
(The notification of the update of the branch is automatic, I think, it
was not a request to retest it right away)

Piotr, can you check the segfault in cosmic reco in the path without HLT
in the meanwhile?

OK, no problems.
I'll wait for a clear "go ahead, it's ready" next time.

Cheers

--slava

Cheers,
Nicola


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


Vyacheslav (Slava) Krutelyov
TAMU: Physics Dept Texas A&M MS4242, College Station, TX 77843-4242
CERN: 42-R-027
AIM/Skype: siava16 googleTalk: [email protected]
(630) 291-5128 Cell (US) +41 76 275 7116 Cell (CERN)



Reply to this email directly or view it on GitHubhttps://github.com//pull/1690#issuecomment-30786250
.

@cmsbuild
Copy link
Contributor

Pull request #1690 was updated. @nclopezo, @danduggan, @rovere, @cmsbuild, @anton-a, @thspeer, @deguio, @slava77, @eliasron can you please check and sign again.

@ptraczyk
Copy link
Contributor

FYI this commit now contains all the modifications, it passes the tests on
my side.

2013/12/19 cmsbuild [email protected]

Pull request #1690 #1690 was
updated. @nclopezo https://github.com/nclopezo, @dandugganhttps://github.com/danduggan,
@rovere https://github.com/rovere, @cmsbuildhttps://github.com/cmsbuild,
@anton-a https://github.com/anton-a, @thspeerhttps://github.com/thspeer,
@deguio https://github.com/deguio, @slava77 https://github.com/slava77,
@eliasron https://github.com/eliasron can you please check and sign
again.

Reply to this email directly or view it on GitHubhttps://github.com//pull/1690#issuecomment-30926392
.

@ptraczyk
Copy link
Contributor

To clarify:
the problem with cosmics and HLT configurations that were crashing the test
jobs has been solved by removing the previously added cfg parameter and
updating the algorithm accordingly. Given this no other changes are
necessary, in particular there is no need to modify confDB, the current HLT
configuration works ok now.
The bug causing the segfaults the other jobs was found and fixed.

2013/12/19 Piotr Traczyk [email protected]

FYI this commit now contains all the modifications, it passes the tests on
my side.

2013/12/19 cmsbuild [email protected]

Pull request #1690 #1690 was
updated. @nclopezo https://github.com/nclopezo, @dandugganhttps://github.com/danduggan,
@rovere https://github.com/rovere, @cmsbuildhttps://github.com/cmsbuild,
@anton-a https://github.com/anton-a, @thspeerhttps://github.com/thspeer,
@deguio https://github.com/deguio, @slava77https://github.com/slava77,
@eliasron https://github.com/eliasron can you please check and sign
again.

Reply to this email directly or view it on GitHubhttps://github.com//pull/1690#issuecomment-30926392
.

@ktf
Copy link
Contributor

ktf commented Dec 20, 2013

To pre12 / 71X.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 6, 2014

@slava77
Copy link
Contributor

slava77 commented Jan 13, 2014

This will go to 71X by default.

@deguio
Copy link
Contributor

deguio commented Jan 21, 2014

+1
this targets 71X. still mergeable.

@ktf
Copy link
Contributor

ktf commented Jan 22, 2014

Closing this. Please reopen in 71X (you can do so using the same branch you used in 70X).

@ktf ktf closed this Jan 22, 2014
ggovi pushed a commit to ggovi/cmssw that referenced this pull request Jan 11, 2017
upgrade lhapdfsets to 6.1.5b with CT14
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.

9 participants