Csc hits

[VinInn]

moved to 7_1_X

took the opportunity to allow cout/cerr in stand-alone software

[cmsbuild]

A new Pull Request was created by @VinInn (Vincenzo Innocente) for CMSSW_7_1_X.

Csc hits

It involves the following packages:

DQM/CSCMonitorModule
DQMOffline/Muon
DataFormats/CSCDigi
DataFormats/MuonData
DataFormats/MuonDetId
EventFilter/CSCRawToDigi
RecoLocalMuon/CSCRecHitD
RecoLocalMuon/CSCValidation

@civanch, @thspeer, @danduggan, @mdhildreth, @cmsbuild, @anton-a, @nclopezo, @rovere, @deguio, @slava77, @Degano, @ojeda 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.
@nclopezo, @ktf you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

[cmsbuild]

+1
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2305/29/summary.html

[civanch]

+1

[slava77]

@ptcox @VinInn
Hi Vincenzo, there were some concerns about changes in the examiner functions that I received from Victor and Tim.
I will send a separate email where we can hopefully converge on a set of changes

[VinInn]

i have introduced a nice #define that will allow a stand alone program to print on cout and cerr.
it is not allowed in production to stream debug output w/o a EDM_LM_DEBUG protection.
no matter how rare on how small its impact is pretented to be.

v.

Sent from my iPad

On Feb 13, 2014, at 22:55, slava77 notifications@github.com wrote:

@ptcox @VinInn
Hi Vincenzo, there were some concerns about changes in the examiner functions that I received from Victor and Tim.
I will send a separate email where we can hopefully converge on a set of changes

—
Reply to this email directly or view it on GitHub.

[Dr15Jones]

Of course cout, cerr are not thread safe.

[ptcox]

How come you’re ‘notifications@github.com’?! All very bizarre. I find github (and git) overly complex.

On Feb 14, 2014, at 14:14, Chris Jones notifications@github.com wrote:

Of course cout, cerr are not thread safe.

—
Reply to this email directly or view it on GitHub.

Already sent the following:

Begin forwarded message:

From: Tim Cox Timothy.Cox@cern.ch
Subject: Re: [cmssw] Csc hits (#2105)
Date: February 14, 2014 at 13:05:06 GMT+1
To: Vincenzo Innocente Vincenzo.Innocente@cern.ch
Cc: Slava Krutelyov slava77@fnal.gov, Victor Barashko barashko@phys.ufl.edu, Anton Anastassov aa@fnal.gov, Victor Barashko Victor.Barachko@cern.ch

Hi Vincenzo, Slava,

That’s pretty neat. Thanks. I’m willing to have these updates/fixes released. The sooner they’re in the sooner we can discover whether there are any minor unexpected side effects.

Regards, Tim

On Feb 14, 2014, at 8:55, Vincenzo Innocente Vincenzo.Innocente@cern.ch wrote:

On 14 Feb, 2014, at 8:18 AM, Vincenzo Innocente vincenzo.innocente@cern.ch wrote:

answered to the github.
I have introduced a nice #define that will allow a stand alone program to print on cout and cerr.
it is not allowed in production to stream debug output w/o a EDM_LM_DEBUG protection.
no matter how rare on how small its impact is pretented to be.

in short, the old code was breaking CMSSW rules.
the new is not.
v.

Sent from my iPad

On Feb 13, 2014, at 22:56, Slava Krutelyov slava77@fnal.gov wrote:

Hi Victor,

Did you have a chance to take a look at the changes in terms of actual
applications?
(beyond just looking at the code).

Since the last email, the changes were moved to 71X release cycle.

I'm adding Vincenzo in cc so that we can converge on a set of changes
that would work for CSC.

Cheers

--slava

On 1/22/14, 11:44 AM, Victor Barashko wrote:
Hi Slava, Tim

I'm currently quite busy with CSC HV upgrade business, but after briefly
looking on proposed changes I have few concerns regarding the
CSCExaminer changes, because
we have at least one standalone expert debugging tool, which relies on
existing code and which is compiled outside of CMSSW environment and I
don't think that
with introduced changes we would be able to recompile it as before.
Also I can not confirm right now if our tools (local DQM, FAST site
analysis), which rely on locally built unpacking code still can be
compiled and work as expected with those changes.
So I wouldn't rush into applying at least the Examiner changes.

best,
Victor

(cut)

[Dr15Jones]

I guess it was because I used the web for for the reply and not my email.

Chris
Sent from my iPad

On Feb 14, 2014, at 7:23 AM, ptcox notifications@github.com wrote:

How come you’re ‘notifications@github.com’?! All very bizarre. I find github (and git) overly complex.

On Feb 14, 2014, at 14:14, Chris Jones notifications@github.com wrote:

Of course cout, cerr are not thread safe.

—
Reply to this email directly or view it on GitHub.

Already sent the following:

Begin forwarded message:

From: Tim Cox Timothy.Cox@cern.ch
Subject: Re: [cmssw] Csc hits (#2105)
Date: February 14, 2014 at 13:05:06 GMT+1
To: Vincenzo Innocente Vincenzo.Innocente@cern.ch
Cc: Slava Krutelyov slava77@fnal.gov, Victor Barashko barashko@phys.ufl.edu, Anton Anastassov aa@fnal.gov, Victor Barashko Victor.Barachko@cern.ch

Hi Vincenzo, Slava,

That’s pretty neat. Thanks. I’m willing to have these updates/fixes released. The sooner they’re in the sooner we can discover whether there are any minor unexpected side effects.

Regards, Tim

On Feb 14, 2014, at 8:55, Vincenzo Innocente Vincenzo.Innocente@cern.ch wrote:

On 14 Feb, 2014, at 8:18 AM, Vincenzo Innocente vincenzo.innocente@cern.ch wrote:

answered to the github.
I have introduced a nice #define that will allow a stand alone program to print on cout and cerr.
it is not allowed in production to stream debug output w/o a EDM_LM_DEBUG protection.
no matter how rare on how small its impact is pretented to be.

in short, the old code was breaking CMSSW rules.
the new is not.
v.

Sent from my iPad

On Feb 13, 2014, at 22:56, Slava Krutelyov slava77@fnal.gov wrote:

Hi Victor,

Did you have a chance to take a look at the changes in terms of actual
applications?
(beyond just looking at the code).

Since the last email, the changes were moved to 71X release cycle.

I'm adding Vincenzo in cc so that we can converge on a set of changes
that would work for CSC.

Cheers

--slava

On 1/22/14, 11:44 AM, Victor Barashko wrote:
Hi Slava, Tim

I'm currently quite busy with CSC HV upgrade business, but after briefly
looking on proposed changes I have few concerns regarding the
CSCExaminer changes, because
we have at least one standalone expert debugging tool, which relies on
existing code and which is compiled outside of CMSSW environment and I
don't think that
with introduced changes we would be able to recompile it as before.
Also I can not confirm right now if our tools (local DQM, FAST site
analysis), which rely on locally built unpacking code still can be
compiled and work as expected with those changes.
So I wouldn't rush into applying at least the Examiner changes.

best,
Victor

(cut)
—
Reply to this email directly or view it on GitHub.

[ptcox]

Ha, but I see I made it to the favoured address too :) It seems nonsensical that github constructs what looks to be an email address, but isn’t.

On Feb 14, 2014, at 14:36, Chris Jones notifications@github.com wrote:

I guess it was because I used the web for for the reply and not my email.

Chris
Sent from my iPad

On Feb 14, 2014, at 7:23 AM, ptcox notifications@github.com wrote:

                            *********************************

How come you’re ‘notifications@github.com’?! All very bizarre. I find github (and git) overly complex.

On Feb 14, 2014, at 14:14, Chris Jones notifications@github.com wrote:

Of course cout, cerr are not thread safe.

—
Reply to this email directly or view it on GitHub.

Already sent the following:

Begin forwarded message:

From: Tim Cox Timothy.Cox@cern.ch
Subject: Re: [cmssw] Csc hits (#2105)
Date: February 14, 2014 at 13:05:06 GMT+1
To: Vincenzo Innocente Vincenzo.Innocente@cern.ch
Cc: Slava Krutelyov slava77@fnal.gov, Victor Barashko barashko@phys.ufl.edu, Anton Anastassov aa@fnal.gov, Victor Barashko Victor.Barachko@cern.ch

Hi Vincenzo, Slava,

That’s pretty neat. Thanks. I’m willing to have these updates/fixes released. The sooner they’re in the sooner we can discover whether there are any minor unexpected side effects.

Regards, Tim

On Feb 14, 2014, at 8:55, Vincenzo Innocente Vincenzo.Innocente@cern.ch wrote:

On 14 Feb, 2014, at 8:18 AM, Vincenzo Innocente vincenzo.innocente@cern.ch wrote:

answered to the github.
I have introduced a nice #define that will allow a stand alone program to print on cout and cerr.
it is not allowed in production to stream debug output w/o a EDM_LM_DEBUG protection.
no matter how rare on how small its impact is pretented to be.

in short, the old code was breaking CMSSW rules.
the new is not.
v.

Sent from my iPad

On Feb 13, 2014, at 22:56, Slava Krutelyov slava77@fnal.gov wrote:

Hi Victor,

Did you have a chance to take a look at the changes in terms of actual
applications?
(beyond just looking at the code).

Since the last email, the changes were moved to 71X release cycle.

I'm adding Vincenzo in cc so that we can converge on a set of changes
that would work for CSC.

Cheers

--slava

On 1/22/14, 11:44 AM, Victor Barashko wrote:
Hi Slava, Tim

I'm currently quite busy with CSC HV upgrade business, but after briefly
looking on proposed changes I have few concerns regarding the
CSCExaminer changes, because
we have at least one standalone expert debugging tool, which relies on
existing code and which is compiled outside of CMSSW environment and I
don't think that
with introduced changes we would be able to recompile it as before.
Also I can not confirm right now if our tools (local DQM, FAST site
analysis), which rely on locally built unpacking code still can be
compiled and work as expected with those changes.
So I wouldn't rush into applying at least the Examiner changes.

best,
Victor

(cut)
—
Reply to this email directly or view it on GitHub.
—
Reply to this email directly or view it on GitHub.

[slava77]

.. testing

[ktf]

Tim, this is done to make sure that your replies go to the correct
thread in the web page of the pull request.

On 14 Feb 2014, at 16:56, slava77 wrote:

.. testing

---

Reply to this email directly or view it on GitHub:
#2305 (comment)

[slava77]

-1

using #2305 27bbeb8
in CMSSW_7_1_X_2014-02-12-1400/sign307

reason: noise from MSG-w in the examiner (see below). If this is a real problem, it should be fixed, if it's false positive, it should be silenced.

Summary:

• physics output is essentially unchanged: there are some numerical-level differences more visible in less numerically stable algos, e.g. btagging IP tag infos, or in histograms with higher numerical sensitivity (e.g. MET in muon gun events).
• technical performance (using matrix wf 20.0 on 2000 events, most modules unrelated to muons/tracks are unchanged; I tried checking 22.0 with 1TeV muons, but there's a bit of disturbance because more numerical uncertainty introduced some toss up in reconstructed objects that's not very relevant to timing estimates)
  • csc2Drechits CPU down by ~30% ( from 7 to 5 sec/job)
• HLT step is now polluted by CSCDCCExaminer: CSCMonitorModule:hltCSCMonitorModule (guessing that it's about one per CSC chamber with hits), rerun a few events with 20.0 or 22.0 to reproduce

%MSG-w CSCDCCExaminer:  CSCMonitorModule:hltCSCMonitorModule  14-Feb-2014 18:09:58 CET Run: 1 Event: 1

DDU Header Occurrence = 1
%MSG
%MSG-w CSCDCCExaminer:  CSCMonitorModule:hltCSCMonitorModule  14-Feb-2014 18:09:58 CET Run: 1 Event: 1
  ERROR 10  ALCT CRC Error                                   
%MSG

[VinInn]

interesting...
i have not seen those warnings in real events.
easy to move to logdebug for what i am concerned... ( one line change #define CERR
take into account that those messages where in the past streamed to " cerr " and silenced only
afterward...
anyhow being this hlt and not reco, i assume that reco is happy with the PR for what it is striclty concerned...
please confim, so that release management can merge sa soon CERR is moved to LogDebug
v.

Sent from my iPad

On Feb 16, 2014, at 0:45, slava77 notifications@github.com wrote:

-1

using #2305 27bbeb8
in CMSSW_7_1_X_2014-02-12-1400/sign307

reason: noise from MSG-w in the examiner (see below). If this is a real problem, it should be fixed, if it's false positive, it should be silenced.

Summary:

physics output is essentially unchanged: there are some numerical-level differences more visible in less numerically stable algos, e.g. btagging IP tag infos, or in histograms with higher numerical sensitivity (e.g. MET in muon gun events).

technical performance (using matrix wf 20.0 on 2000 events, most modules unrelated to muons/tracks are unchanged; I tried checking 22.0 with 1TeV muons, but there's a bit of disturbance because more numerical uncertainty introduced some toss up in reconstructed objects that's not very relevant to timing estimates)

csc2Drechits CPU down by ~30% ( from 7 to 5 sec/job)
HLT step is now polluted by CSCDCCExaminer: CSCMonitorModule:hltCSCMonitorModule (guessing that it's about one per CSC chamber with hits), rerun a few events with 20.0 or 22.0 to reproduce

%MSG-w CSCDCCExaminer: CSCMonitorModule:hltCSCMonitorModule 14-Feb-2014 18:09:58 CET Run: 1 Event: 1

DDU Header Occurrence = 1
%MSG
%MSG-w CSCDCCExaminer: CSCMonitorModule:hltCSCMonitorModule 14-Feb-2014 18:09:58 CET Run: 1 Event: 1
ERROR 10 ALCT CRC Error
%MSG

—
Reply to this email directly or view it on GitHub.

[VinInn]

to make regression happy, LogWarning has been degraded to LogDebug
this restore the previous behavior (they were streamed to LogDebug in the client).
I am not fully satisfy to this solution:
in other cases (for instance DT) similar warnings were sign of miss configuration, wrong global tag.
a CRC error in other detectors is fatal.
I would suggest that experts (such as @ptcox , Victor and @fwyzard ) have a look, in their respective area of expertise, to confirm that these messages are actually false positive and do not hint to a more serious defect
@slava77 , in which workflow did you observe the messages?

My understanding is that this were the only defect found.
I think @ktf can merge in IB now.

[cmsbuild]

Pull request #2305 was updated. @civanch, @ojeda, @vlimant, @davidlange6, @franzoni, @mdhildreth, @monttj, @cmsbuild, @anton-a, @thspeer, @rovere, @deguio, @slava77, @danduggan, @vadler, @Degano, @nclopezo can you please check and sign again.

[fwyzard]

well, I definitely do not consider myself an expert on the CSC errors :-(

@ptcox , what do you think about the ERROR 10 ALCT CRC Error mentioned by Slava ?

.A

[ptcox]

I'm not an expert either - I've always tried to avoid becoming an expert in the CSC raw event format :)
My first guess would be that this running on simulated data and that is a long-standing limitation of the DigiToRaw Packer that until now has been swept under the carpet. The raw event format is complex and the DigiToRaw version has always been an approximation. This could be one of the details that has never been tweaked.

So this leads to a number of questions

• Is this a new error? Or just an old error has now been made visible by Vincenzo's updates? I don't think Vincenzo's updates should have affected the operation of the Examiner so I hope this is just a 'visibility' issue.
• IS this using simulated data? If it is with real data then something has been broken in the Examiner's functionality, and that is a serious issue.

In either case I presume the fact that appears in HLT is nothing to do with HLT since the CSC Unpacker has no special HLT vs Offline mode of operation, AFAIK.

Regards, Tim

---

From: Andrea Bocci [notifications@github.com]
Sent: 16 February 2014 13:17
To: cms-sw/cmssw
Cc: Tim Cox
Subject: Re: [cmssw] Csc hits (#2305)

well, I definitely do not consider myself an expert on the CSC errors :-(

@ptcoxhttps://github.com/ptcox , what do you think about the CRC Error mentioned by Slava ?

.A
.A

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2305#issuecomment-35195345.

[VinInn]

On 16 Feb, 2014, at 2:44 PM, ptcox notifications@github.com wrote:

So this leads to a number of questions

• Is this a new error? Or just an old error has now been made visible by Vincenzo's updates? I don't think Vincenzo's updates should have affected the operation of the Examiner so I hope this is just a 'visibility' issue.
  Indeed: in the old code it was not visible (not sure if it was counted, for sure was not verified…)
• IS this using simulated data?
  yes for instance in 22.0_SingleMuPt1000+SingleMuPt1000INPUT+DIGI+RECO+HARVEST
  If it is with real data then something has been broken in the Examiner's functionality, and that is a serious issue.

Never seen in data (neither hlt nor reco, never seen during reconstruction of simulated events either.
In either case I presume the fact that appears in HLT is nothing to do with HLT since the CSC Unpacker has no special HLT vs Offline mode of operation, AFAIK.

It comes form an ALCA/DQM module usually heavily prescaled in operation
v.

[slava77]

I've seen this error only in MC.

[slava77]

..uhm, #2305 529c011
is much more fresh, mixed up with merged stuff from a very recent 71X head.
It would've been nicer to see just one extra commit, instead of a few pages of them.
This is mainly a visual annoyance, browsing the changes on github web interface.

[slava77]

(well, it's more than a visual annoyance: I can't cleanly merge this onto my old test area)

[VinInn]

sorry slava,
a pr has to eventually merge into the most recent IB.
it was impossible to use an IB a week old.
in any case i changed only two lines. no more regression.
the cause is understood. it did not affect reco anyhow.
could you please say if reco is satiflied?
as Tim said, CSC DPG will than take care to validate the other use cases

v.

Sent from my iPad

On Feb 16, 2014, at 22:56, slava77 notifications@github.com wrote:

..uhm, #2305 529c011
is much more fresh, mixed up with merged stuff from a very recent 71X head.
It would've been nicer to see just one extra commit, instead of a few pages of them.
This is mainly a visual annoyance, browsing the changes on github web interface.

—
Reply to this email directly or view it on GitHub.

[VinInn]

[innocent@vinavx2 src]$ git rebase -i
Successfully rebased and updated refs/heads/from-CMSSW_7_1_X_2014-02-15-1400.
[innocent@vinavx2 src]$ git push my-cmssw HEAD:CSCHits -f
Counting objects: 719, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (278/278), done.
Writing objects: 100% (426/426), 123.34 KiB | 0 bytes/s, done.
Total 426 (delta 338), reused 202 (delta 142)
To git@github.com:VinInn/cmssw.git

• 529c011...1c3301a HEAD -> CSCHits (forced update)

hope fixes the issue...

[cmsbuild]

Pull request #2305 was updated. @civanch, @thspeer, @danduggan, @mdhildreth, @cmsbuild, @anton-a, @nclopezo, @rovere, @deguio, @slava77, @Degano, @ojeda can you please check and sign again.

[cmsbuild]

+1
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2305/191/summary.html

[slava77]

Thanks.
The history is clean.

(sadly, it only made it worse for my reference test area. I prefer to keep my tests clean and have a reference test area that corresponds to the signed off/merged state of the PR. This rebase is made on an even more fresh IB than the last fresh I was making. You don't need to make any more changes here.)

[slava77]

+1

tested #2305 1c3301a
in CMSSW_7_1_X_2014-02-17-0200/sign307a

Diffs in physics content are summarized above in #2305 (comment)
The EDM warnings are now gone, and the extra events skimmed by the LogMonitor are gone as well.

[civanch]

+1

[VinInn]

why this has not been merged yet?

[davidlange6]

needs a dqm signature. @deguio?

[VinInn]

On 21 Feb, 2014, at 8:39 AM, davidlange6 notifications@github.com wrote:

needs a dqm signature. @deguio?

for what DQM is concerned
DQMOffline/Muon/src/CSCOfflineMonitor.cc
is not affected as
the two examiner_out xaminer_err where never used (I forgot to delete, being objects c++ unused variable does not catch them…

in principle
the behavioiur of
DQM/CSCMonitorModule/plugins/CSCDQM_EventProcessor_processEvent.cc
is modified (now requires LogDebug to be active, before was controlled by a config->getBINCHECKER_OUTPUT()

in the current implementation there is no way to re-establish thie previous behaviour w/o having the CSCDCCExaminer to stream w/o any control (which is agianst the rule)
So the CSCMonitorModule (sorry the CSCDCCExaminer) has to be recompiled with LogDebug if debug output is required.
This could be the subject of a new PR later on by DQM experts (once they agree on what is really needed and how to implement it w/o impacting HLT performances.

Let me stress that the code for HLT and Reco shall NOT be compiled with LogDebug enabled.

v.

[davidlange6]

ok -I suggest we bypass for the moment so this ends up in pre3.

On Feb 21, 2014, at 8:56 AM, Vincenzo Innocente notifications@github.com
wrote:

On 21 Feb, 2014, at 8:39 AM, davidlange6 notifications@github.com wrote:

needs a dqm signature. @deguio?

for what DQM is concerned
DQMOffline/Muon/src/CSCOfflineMonitor.cc
is not affected as
the two examiner_out xaminer_err where never used (I forgot to delete, being objects c++ unused variable does not catch them…

in principle
the behavioiur of
DQM/CSCMonitorModule/plugins/CSCDQM_EventProcessor_processEvent.cc
is modified (now requires LogDebug to be active, before was controlled by a config->getBINCHECKER_OUTPUT()

in the current implementation there is no way to re-establish thie previous behaviour w/o having the CSCDCCExaminer to stream w/o any control (which is agianst the rule)
So the CSCMonitorModule has to be recompiled with LogDebug if debug output is required.
This could be the subject of a new PR later on by DQM experts (once they agree on what is really needed and how to implement it w/o impacting HLT performances.

v.
—
Reply to this email directly or view it on GitHub.