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

HBHE M2 = HPD old code cleanup #17680

Merged
merged 2 commits into from Mar 21, 2017
Merged

Conversation

mariadalfonso
Copy link
Contributor

Given that we are switching the legacy re-reco to the "Phase1"code with the #17676.
I also removed the various if statement for the HPD-QIE8.

Now take the noise from the database instead of a flat numbers from the python code
the ADCquantization noise use the exact definition instead of a parametrized version

expect minor changes in the noise hits

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 2, 2017

A new Pull Request was created by @mariadalfonso for master.

It involves the following packages:

RecoLocalCalo/HcalRecAlgos

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

cms-bot commands are listed here #13028

@slava77
Copy link
Contributor

slava77 commented Mar 2, 2017

@mariadalfonso
if I understand correctly, this PR changes behavior and needs #17676 to be merged to proceed with tests.
Please confirm.
Thank you.

@abdoulline
Copy link

90X #17676 -> now 91X #17688 for this PR

@mariadalfonso
Copy link
Contributor Author

@slava77
To see the effect of this PR on some of the workflow we do not need to wait for the #17676 ; but this PR should be merged only if the #17676, that is when the completely dismiss the "run1/run2-2016" code in favor of the "Phase1" for all the era.

@slava77
Copy link
Contributor

slava77 commented Mar 2, 2017

@cmsbuild please test
.. to be re-tested after #17676

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 2, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/18064/console Started: 2017/03/02 15:32
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/18109/console Started: 2017/03/03 03:01

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 3, 2017

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 3, 2017

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 3, 2017

@mariadalfonso
Copy link
Contributor Author

@slava77
What I do with this PR, should I forward port to the 91X (as the 17676 ->17688) ?

@slava77
Copy link
Contributor

slava77 commented Mar 8, 2017

@mariadalfonso this is already a 91X PR.
I didn't get to #17688 yet.
Once it's signed off, this PR will be reviewed.

@slava77
Copy link
Contributor

slava77 commented Mar 18, 2017

@cmsbuild please test

the last set of tests has expired

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 18, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/18535/console Started: 2017/03/18 05:10

@slava77
Copy link
Contributor

slava77 commented Mar 20, 2017

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 20, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/18570/console Started: 2017/03/20 20:42

@slava77
Copy link
Contributor

slava77 commented Mar 20, 2017

@kpedro88 @abdoulline
maybe you can comment on the difference in chi2 between HB and HE
is digitization/quantization scale different or something else similar in effect on the pulse shape

Looking vs energy, it doesn't appear as if the effect is noise related
(the plot is from wf 10071, 2017 flat-pt QCD )
wf10071_chi2_vs_e_hb
wf10071_chi2_vs_e_he

@mariadalfonso
Copy link
Contributor Author

mariadalfonso commented Mar 20, 2017

@slava77
This PR takes

  1. ADC quantization from exact definitions, some test can be found in HBHE: use proper quantization noise in the M2 #17037
  2. pedestal width from the DB HBHE M2: noise definition cleanup #17129

From plots in these PR, I do not expect changes from 1) and 2) in the chi2

  1. change also the gaussian constraint on the pedestal

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@abdoulline
Copy link

abdoulline commented Mar 20, 2017 via email

@cmsbuild
Copy link
Contributor

@mariadalfonso
Copy link
Contributor Author

@abdoulline

(1) pfotostatistics. Factor ~1.8 more light (p.e.) in HB for the same
"end" RecHit energy. So, relative photostatistics error is bigger in HE [*]

This PhotoStat term didn't change in this PR, as was already active in the release for both the HB and HE and HEP17

(2) similarly ADC quantization scale is lower in HE by factor ~1.8.
less fC = lower ADC scale for the same "end" RecHit energy wrt HB -
means bigger relative quantization error in HE.

Well, it might mean that if something is changed, Chi2 effect should be
less pronounced in HE (with its bigger error terms) ?

S.

@abdoulline
Copy link

abdoulline commented Mar 21, 2017 via email

@slava77
Copy link
Contributor

slava77 commented Mar 21, 2017

+1

for #17680 9eb66e9

#17680 (comment) :
changes are small enough to not affect whatever lower level calibrations are to be done with 90X. So, I think that this can go to 91X.

@cmsbuild
Copy link
Contributor

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

@davidlange6
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 044a3f4 into cms-sw:master Mar 21, 2017
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