Mostly changes related to handling TDC info #14920

igv4321 · 2016-06-19T16:10:08Z

HFQIE10Info and rechit timing is now in ns instead of TDC units.
Correct handling of special QIE10 TDC values 62 and 63.
Made provisions for proper handing of single-anode PMTs in the mixed-readout scenario.
Use "sample-of-interest" time slice for setting "HARDWARE_ERROR" algorithm status.

The code is checked with the Phase 1 recipe described at
https://twiki.cern.ch/twiki/bin/viewauth/CMS/HcalPhase1SoftwareSimulationRecipe

cmsbuild · 2016-06-19T16:10:32Z

A new Pull Request was created by @igv4321 (Igor Volobouev) for CMSSW_8_1_X.

It involves the following packages:

DataFormats/HcalRecHit
RecoLocalCalo/HcalRecAlgos
RecoLocalCalo/HcalRecProducers

@cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@argiro this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

slava77 · 2016-06-19T16:17:53Z

DataFormats/HcalRecHit/interface/HFQIE10Info.h

+
+    // Special value for the rise time used in case the QIE10 pulse
+    // is always above the discriminator
+    static constexpr float UNKNOWN_T_OVERSHOOT = -101.f;


overshoot is smaller than undershoot?

These are just technical numbers. The only requirements
for them is that they are exactly representable floats, and
that they can not be encountered during normal readouts.
It is also nice to define them in such a way that, if desired,
they can be seen in plots together with "normal" readouts.

On 06/19/2016 11:17 AM, Slava Krutelyov wrote:

In DataFormats/HcalRecHit/interface/HFQIE10Info.h #14920 (comment):

@@ -21,6 +21,14 @@ class HFQIE10Info
static const unsigned N_RAW_MAX = 5;
static const raw_type INVALID_RAW = std::numeric_limits<raw_type>::max();

// Special value for the rise time used in case the QIE10 pulse

// is always below the discriminator

static constexpr float UNKNOWN_T_UNDERSHOOT = -100.f;

// Special value for the rise time used in case the QIE10 pulse

// is always above the discriminator

static constexpr float UNKNOWN_T_OVERSHOOT = -101.f;

overshoot is smaller than undershoot?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub https://github.com/cms-sw/cmssw/pull/14920/files/0b14bc4b41a8878079e1ed29025e58c078e6f68a#r67619481, or mute the thread
https://github.com/notifications/unsubscribe/AFS4-Ypcq6z48vojFrUR7sY5QkcmfNaEks5qNWu0gaJpZM4I5Mg7.

thanks for the explanation
( just magic numbers )

slava77 · 2016-06-19T16:20:28Z

@cmsbuild please test

cmsbuild · 2016-06-19T16:20:48Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/13586/console

cmsbuild · 2016-06-19T19:07:26Z

+1
Tested at: 0b14bc4
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-14920/13586/summary.html

cmsbuild · 2016-06-19T20:29:51Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-14920/13586/summary.html

…pping

cmsbuild · 2016-06-22T16:57:56Z

Pull request #14920 was updated. @cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please check and sign again.

slava77 · 2016-06-22T17:06:10Z

@cmsbuild please test
... a positive magic number would be visible even better

cmsbuild · 2016-06-22T17:06:27Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/13628/console

igv4321 · 2016-06-22T17:15:14Z

Maybe, but with negative magic numbers there can be no
confusion about their specialness

On 06/22/2016 12:06 PM, Slava Krutelyov wrote:

@cmsbuild https://github.com/cmsbuild please test
... a positive magic number would be visible even better

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #14920 (comment), or mute the thread
https://github.com/notifications/unsubscribe/AFS4-c-TNlDj4L_5HSI6d4hJFSB6uT3Dks5qOWuFgaJpZM4I5Mg7.

…ckage

cmsbuild · 2016-06-25T07:58:44Z

Pull request #14920 was updated. @cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please check and sign again.

slava77 · 2016-06-25T15:53:38Z

@cmsbuild please test

cmsbuild · 2016-06-25T15:53:57Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/13676/console

cmsbuild · 2016-06-25T18:19:18Z

+1
Tested at: d69aae5
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-14920/13676/summary.html

cmsbuild · 2016-06-25T19:18:25Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-14920/13676/summary.html

cvuosalo · 2016-06-27T12:17:09Z

RecoLocalCalo/HcalRecAlgos/src/HFSimpleTimeCheck.cc

+                    hardwareOK = false;
+            }
+        }
+        if (!hardwareOK)


Code duplication can be avoided here, starting at line 39:

unsigned startSample = sampleToCheck, endSample = sampleToCheck + 1; if (sampleToCheck >= nRaw) { startSample = 0; endSample = nRaw; } for (sampleToCheck = startSample; sampleToCheck < endSample; ++sampleToCheck) { const QIE10DataFrame::Sample s(anode.getRaw(sampleToCheck)); if (!s.ok()) hardwareOK = false; }

Sorry, which code duplication? I prefer to write my code
so that the least possible amount of work is performed
in the branch that is encountered most often.

On 06/27/2016 07:17 AM, Carl Vuosalo wrote:

In RecoLocalCalo/HcalRecAlgos/src/HFSimpleTimeCheck.cc #14920 (comment):

{

const QIE10DataFrame::Sample s(anode.getRaw(sampleToCheck));

hardwareOK = s.ok();

}

else

{

// This should not normally happen, but we still

// need to do something reasonable here

for (sampleToCheck = 0; sampleToCheck < nRaw; ++sampleToCheck)

{

const QIE10DataFrame::Sample s(anode.getRaw(sampleToCheck));

if (!s.ok())

hardwareOK = false;

}

}

if (!hardwareOK)

Code duplication can be avoided here, starting at line 39:

|unsigned startSample = sampleToCheck, endSample = sampleToCheck + 1; if (sampleToCheck >= nRaw) { startSample = 0; endSample = nRaw; } for (sampleToCheck = startSample; sampleToCheck < endSample;
++sampleToCheck) { const QIE10DataFrame::Sample s(anode.getRaw(sampleToCheck)); if (!s.ok()) hardwareOK = false; } |

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub https://github.com/cms-sw/cmssw/pull/14920/files/d69aae5b5dd9e76e1ba10f46e8ed58dd93044926#r68566046, or mute the thread
https://github.com/notifications/unsubscribe/AFS4-S7Y0LuphBdB-mu-lOI5IQC3_rpDks5qP79IgaJpZM4I5Mg7.

@igv4321: OK. I was just making a suggestion. If this method is called at very high frequency, then performance considerations take priority.

cvuosalo · 2016-06-27T13:42:24Z

@igv4321: What option to SinglePi_Phase1.sh is best to test this PR? Is -m 1 best?

igv4321 · 2016-06-27T14:11:15Z

No, use either -m 2 or -m 3.

On 06/27/2016 08:42 AM, Carl Vuosalo wrote:

@igv4321 https://github.com/igv4321: What option to |SinglePi_Phase1.sh| is best to test this PR? Is |-m 1| best?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #14920 (comment), or mute the thread
https://github.com/notifications/unsubscribe/AFS4-V1NLWikr-jtBdOGZ3aDhNkHTUrMks5qP9NDgaJpZM4I5Mg7.

cvuosalo · 2016-06-28T14:21:31Z

+1

For #14920 d69aae5

Revising usage of TDC info for Phase 1 HF. There should be no change in monitored quantities.

The code changes are satisfactory, and Jenkins tests against baseline CMSSW_8_1_X_2016-06-25-1100 show no significant differences, as expected. A custom Phase 1 HCAL test was performed (using SinglePi_Phase1.sh -m 2) against baseline CMSSW_8_1_X_2016-06-19-2300 shows no differences and only a marginal increase in CPU time (excluding the first event):

  delta/mean delta/orJob     original                   new        module name
  ---------- ------------     --------                  ----       ------------
   +1.860135      +0.09%         0.03 ms/ev ->          0.86 ms/ev particleFlowRecHitHF
   +1.518749      +0.01%         0.02 ms/ev ->          0.12 ms/ev particleFlowClusterHF
   +1.021881      +0.06%         0.27 ms/ev ->          0.84 ms/ev hfreco
  ---------- ------------     --------                  ----       ------------
Job total:  0.928222 s/ev ==> 0.936194 s/ev

RECO and Mini-AOD event content increases in size somewhat:

RECO
-----------------------------------------------------------------
   or, B         new, B      delta, B   delta, %   deltaJ, %    branch 
-----------------------------------------------------------------
    842.8 ->       995.2        152     16.6   0.04     recoPFBlocks_particleFlowBlock__RECO.
   8411.0 ->     14669.6       6259     54.2   1.47     CaloTowersSorted_towerMaker__RECO.
    245.2 ->     11417.8      11173     191.6  2.63     HFRecHitsSorted_hfreco__RECO.
   3810.0 ->      4576.6        767     18.3   0.18     recoPFCandidates_particleFlow__RECO.
    247.0 ->      1088.4        841     126.0  0.20     recoPFClusters_particleFlowClusterHF__RECO.

-------------------------------------------------------------
   424335 ->      443744      19409             4.6     ALL BRANCHES

Mini-AOD
-----------------------------------------------------------------
   or, B         new, B      delta, B   delta, %   deltaJ, %    branch 
-----------------------------------------------------------------
   3297.5 ->      3452.4        155      4.6   0.06     patPackedCandidates_packedPFCandidates__RECO.
-------------------------------------------------------------
   250781 ->      250960        179             0.1     ALL BRANCHES

cmsbuild · 2016-06-28T14:21:37Z

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

davidlange6 · 2016-06-29T08:23:39Z

+1

Mostly changes related to handling TCD info

0b14bc4

cmsbuild added this to the Next CMSSW_8_1_X milestone Jun 19, 2016

cmsbuild added reconstruction-pending pending-signatures tests-pending orp-pending comparison-pending labels Jun 19, 2016

slava77 reviewed Jun 19, 2016
View reviewed changes

cmsbuild added tests-started and removed tests-pending labels Jun 19, 2016

cmsbuild added tests-approved and removed tests-started labels Jun 19, 2016

cmsbuild added comparison-available and removed comparison-pending labels Jun 19, 2016

using a magic number which is easier to see on the plots without rema…

0dbe70d

…pping

cmsbuild added tests-pending comparison-pending and removed comparison-available tests-approved labels Jun 22, 2016

cmsbuild added tests-started and removed tests-pending labels Jun 22, 2016

moving parseHFPhase1AlgoDescription function to a more appropriate pa…

d69aae5

…ckage

cmsbuild added tests-pending comparison-pending and removed comparison-available tests-approved labels Jun 25, 2016

cmsbuild added tests-started and removed tests-pending labels Jun 25, 2016

cmsbuild added tests-approved and removed tests-started labels Jun 25, 2016

cmsbuild added comparison-available and removed comparison-pending labels Jun 25, 2016

cvuosalo reviewed Jun 27, 2016
View reviewed changes

cmsbuild added reconstruction-approved fully-signed and removed pending-signatures reconstruction-pending labels Jun 28, 2016

igv4321 mentioned this pull request Jun 28, 2016

Changing linkage of the "timeshift_ns_hbheho" function to external #14957

Merged

cmsbuild added orp-approved and removed orp-pending labels Jun 29, 2016

cmsbuild merged commit 92706c2 into cms-sw:CMSSW_8_1_X Jun 29, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mostly changes related to handling TDC info #14920

Mostly changes related to handling TDC info #14920

igv4321 commented Jun 19, 2016

cmsbuild commented Jun 19, 2016

slava77 Jun 19, 2016

igv4321 Jun 19, 2016

slava77 Jun 19, 2016

slava77 commented Jun 19, 2016

cmsbuild commented Jun 19, 2016 •

edited

cmsbuild commented Jun 19, 2016

cmsbuild commented Jun 19, 2016

cmsbuild commented Jun 22, 2016

slava77 commented Jun 22, 2016

cmsbuild commented Jun 22, 2016 •

edited

igv4321 commented Jun 22, 2016

cmsbuild commented Jun 25, 2016

slava77 commented Jun 25, 2016

cmsbuild commented Jun 25, 2016 •

edited

cmsbuild commented Jun 25, 2016

cmsbuild commented Jun 25, 2016

cvuosalo Jun 27, 2016

igv4321 Jun 27, 2016

cvuosalo Jun 27, 2016

cvuosalo commented Jun 27, 2016

igv4321 commented Jun 27, 2016

cvuosalo commented Jun 28, 2016

cmsbuild commented Jun 28, 2016

davidlange6 commented Jun 29, 2016

Mostly changes related to handling TDC info #14920

Mostly changes related to handling TDC info #14920

Conversation

igv4321 commented Jun 19, 2016

cmsbuild commented Jun 19, 2016

slava77 Jun 19, 2016

Choose a reason for hiding this comment

igv4321 Jun 19, 2016

Choose a reason for hiding this comment

slava77 Jun 19, 2016

Choose a reason for hiding this comment

slava77 commented Jun 19, 2016

cmsbuild commented Jun 19, 2016 • edited

cmsbuild commented Jun 19, 2016

cmsbuild commented Jun 19, 2016

cmsbuild commented Jun 22, 2016

slava77 commented Jun 22, 2016

cmsbuild commented Jun 22, 2016 • edited

igv4321 commented Jun 22, 2016

cmsbuild commented Jun 25, 2016

slava77 commented Jun 25, 2016

cmsbuild commented Jun 25, 2016 • edited

cmsbuild commented Jun 25, 2016

cmsbuild commented Jun 25, 2016

cvuosalo Jun 27, 2016

Choose a reason for hiding this comment

igv4321 Jun 27, 2016

Choose a reason for hiding this comment

cvuosalo Jun 27, 2016

Choose a reason for hiding this comment

cvuosalo commented Jun 27, 2016

igv4321 commented Jun 27, 2016

cvuosalo commented Jun 28, 2016

cmsbuild commented Jun 28, 2016

davidlange6 commented Jun 29, 2016

cmsbuild commented Jun 19, 2016 •

edited

cmsbuild commented Jun 22, 2016 •

edited

cmsbuild commented Jun 25, 2016 •

edited