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

HCAL Method2 BX fix and timing adjustment #7492

Merged
merged 5 commits into from Feb 6, 2015

Conversation

kfiekas
Copy link
Contributor

@kfiekas kfiekas commented Feb 1, 2015

Small bug in BX calculation in Method2 has been fixed. In addition, the returned time from Method2 has been changed to real time and is now synchronized with Method0 timing.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 1, 2015

A new Pull Request was created by @kfiekas for CMSSW_7_3_X.

HCAL Method2 BX fix and timing adjustment

It involves the following packages:

RecoLocalCalo/HcalRecAlgos
RecoLocalCalo/HcalRecProducers

@cmsbuild, @cvuosalo, @nclopezo, @slava77 can you please review it and eventually sign? Thanks.
@argiro 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.

@slava77
Copy link
Contributor

slava77 commented Feb 1, 2015

@cmsbuild please test

I will also run larger stat tests locally

@slava77
Copy link
Contributor

slava77 commented Feb 1, 2015

@kfiekas
please submit a matching PR to 74X

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 1, 2015

The tests are being triggered in jenkins.

@slava77
Copy link
Contributor

slava77 commented Feb 1, 2015

unlike in the last update in Method2, the auto-forward port will not work (the changes here are in parts of the code that was changed between 73X and 74X).

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 1, 2015

@slava77
Copy link
Contributor

slava77 commented Feb 1, 2015

@bachtis @ahinzmann @abdoulline @violatingcp
please check/confirm that these changes are what was expected from the Friday meeting (or is there more change necessary)
Thank you.

Some plots with this PR would be helpful as well to document what to expect

@slava77
Copy link
Contributor

slava77 commented Feb 4, 2015

I looked a bit in detail at what's happening in cases with large changes in hit energy.

  • the most significant conclusion is that I didn't find a strong correlation between the large pfMet changes and large changes in hit energies (my plots above were pfMet). So, this is good.
  • the migration of hits from 0 to some value above 1 compared to the other way is about 5:1 (167 hits went from ~0 to > 1, compared to 34 the other way around)

Before signing off, please confirm (Salavat or Stephanie or Katharina?) that these changes make sense:
the first two is just root tree indices; then there are 4 ints with raw ADC and the capId from the hits, the new energy, the old energy, ieta, depth, and new and old caloMETs.
I selected these from the QCD flat-pt dijet sample with a deltaE threshold of 20 GeV .

* eventInd *   hitInd *   ADC1    *    ADC2   *    ADC3   *    ADC4   *    CAPID  *   ADC1    *    ADC2   *    ADC3   *    ADC4   *    CAPID  * E new     *    E old  *    iEta   *   depth   * calMETnew * calMETold *
*       28 *      447 *         4 *         4 *         1 *         5 *         0 *        35 *        18 *        12 *         7 *         0 * 23.184209 * 1.125e-06 *        19 *         2 * 15.830185 * 17.149618 *
*      133 *      488 *         3 *         2 *         5 *         9 *         3 *        47 *        26 *        15 *         8 *         3 * 52.381126 * 14.513108 *        28 *         2 * 14.894344 * 17.204561 *
*      404 *      451 *         3 *         2 *         3 *        17 *         2 *        61 *        36 *        22 *        13 *         2 * 156.48962 * 120.68462 *       -21 *         1 * 48.499824 * 54.593601 *
*      407 *      495 *         2 *         3 *         3 *         4 *         1 *        28 *        18 *        12 *         5 *         1 * 21.472896 * 0.6769155 *       -27 *         3 * 13.523726 * 12.239128 *


@slava77
Copy link
Contributor

slava77 commented Feb 5, 2015

One of these helped me to understand better: I selected hits with a change in energy which have exactly the same ADC values in 3,4,5,6 as the one that had a large change from previously large value to now small value.
The second hit below could've probably been picked up as in-time with extra convincing of the fit that it's really a no-pileup (or even 50 ns); with an assumption that this is 25 ns spaced hits, then the new value of zero also makes sense.

*      385 *      364 *         3 *         2 *         4 *         2 *         2 *        17 *        13 *         8 * 3.821e-12 * 1.968e-08 *       -16 *         2 * 18.777298 * 22.045057 *
*      548 *      267 *         3 *         3 *         4 *         2 *         2 *        17 *         7 *         5 * 4.049e-08 * 5.4289159 *        11 *         1 * 5.7206263 * 9.6728849 *


@slava77
Copy link
Contributor

slava77 commented Feb 5, 2015

+1

for #7492 a52b42a

tested in CMSSW_7_3_1_patch2 /test area sign731p2a/
signed based on the discussion above

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 5, 2015

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

@abdoulline
Copy link

Thank you for the detailed scrutiny, Slava.

Yesterday night wasn't able to look at the things. Just would like to
mention we have 8 TS (Digi) stored for RecHits in 2 aux.words and I was
looking at every suspicious rechit's digi array.
In addition: Katharina and I, we don't observe (anymore) rare
big-good-shape signals falling into "peak 0". This was a primary goal of
this fix.

Salavat

On Thu, 5 Feb 2015, Slava Krutelyov wrote:

One of these helped me to understand better: I selected hits with a change in energy which have exactly the same ADC
values in 3,4,5,6 as the one that had a large change from previously large value to now small value.
The second hit below could've probably been picked up as in-time with extra convincing of the fit that it's really a
no-pileup (or even 50 ns); with an assumption that this is 25 ns spaced hits, then the new value of zero also makes
sense.

  •  385 \*      364 \*         3 \*         2 \*         4 \*         2 \*         2 \*        17 \*        13 \* 
    
    8 * 3.821e-12 * 1.968e-08 * -16 * 2 * 18.777298 * 22.045057 *
  •  548 \*      267 \*         3 \*         3 \*         4 \*         2 \*         2 \*        17 \*         7 \* 
    
    5 * 4.049e-08 * 5.4289159 * 11 * 1 * 5.7206263 * 9.6728849 *


Reply to this email directly or view it on GitHub.[AEx02iz2ZY1YyIVPCntdMfiTvigqb0LVks5norEDgaJpZM4DaFF8.gif]

@slava77
Copy link
Contributor

slava77 commented Feb 5, 2015

Salavat,

For me, in this scrutiny the "deal maker" was the confirmation of
small/no correlation between the large diff in HCAL hit energy and the
pfMet values which bothered me the most.

The details of the fit values vs TS ADC values was more educational.

Slava

On 2/4/15 11:43 PM, abdoulline wrote:

Thank you for the detailed scrutiny, Slava.

Yesterday night wasn't able to look at the things. Just would like to
mention we have 8 TS (Digi) stored for RecHits in 2 aux.words and I was
looking at every suspicious rechit's digi array.
In addition: Katharina and I, we don't observe (anymore) rare
big-good-shape signals falling into "peak 0". This was a primary goal of
this fix.

Salavat

On Thu, 5 Feb 2015, Slava Krutelyov wrote:

One of these helped me to understand better: I selected hits with a change in energy which have exactly the same ADC
values in 3,4,5,6 as the one that had a large change from previously large value to now small value.
The second hit below could've probably been picked up as in-time with extra convincing of the fit that it's really a
no-pileup (or even 50 ns); with an assumption that this is 25 ns spaced hits, then the new value of zero also makes
sense.

  •  385 \*      364 \*         3 \*         2 \*         4 \*         2 \*         2 \*        17 \*        13 \* 
    
    8 * 3.821e-12 * 1.968e-08 * -16 * 2 * 18.777298 * 22.045057 *
  •  548 \*      267 \*         3 \*         3 \*         4 \*         2 \*         2 \*        17 \*         7 \* 
    
    5 * 4.049e-08 * 5.4289159 * 11 * 1 * 5.7206263 * 9.6728849 *


Reply to this email directly or view it on GitHub.[AEx02iz2ZY1YyIVPCntdMfiTvigqb0LVks5norEDgaJpZM4DaFF8.gif]


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


@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

6 participants