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 25ns constrained Method 2 for pre3 #6432

Merged
merged 19 commits into from Nov 19, 2014

Conversation

lihux25
Copy link
Contributor

@lihux25 lihux25 commented Nov 16, 2014

Update on Nov 18th 13:00PM (Chicago time)
] Customize different methods

  1. Default is method 2 -> this should be tested
  2. To get method 1 -> --customise RecoLocalCalo/HcalRecProducers/HBHE_custom_25nsMethod.customise_HBHE_Method1
  3. To get method 0 -> --customise RecoLocalCalo/HcalRecProducers/HBHE_custom_25nsMethod.customise_HBHE_Method0

Update on Nov 17th 17:30PM (Chicago time)
] Incorporate comments from Stoyan
] Switch from 3 iterations of the fit to 1 iteration by default
] Fix two problems reported from static analyzer

  1. Fix the pulse shape used for MC to be number 105
  2. If method 2 fails (nan issue) or not used (e.g., tstrig < ts4Min), it falls back now to Method 0
  3. Add an option to control number of fit iterations
  4. Separate out the hbheNegativeFlagSetter from the puCorrMethod switch. It was switched off when Method 1 was disabled. This hbheNegaviteFlagSetter is controlled by setNegativeFlags_ anyway.
  5. Add print out to what method is actually activated

violatingcp and others added 14 commits November 7, 2014 21:33
Conflicts:
	RecoLocalCalo/HcalRecAlgos/interface/PulseShapeFitOOTPileupCorrection.h
	RecoLocalCalo/HcalRecAlgos/src/PulseShapeFitOOTPileupCorrection.cc
1. Fix the pulseshape for MC used
2. Add options to control number of fit times
3. hbheNegativeFlagSetter is seperated out from the puCorrMethod switch (it's controlled by setNegativeFlags_ anyway)
4. Add print out to what method is actually activated
] Fall back to method 0 if fit fails or not performed.
@cmsbuild
Copy link
Contributor

A new Pull Request was created by @lihux25 (Hongxuan Liu) for CMSSW_7_3_X.

Hcal 25ns constrained Method 2 for pre3

It involves the following packages:

RecoLocalCalo/HcalRecAlgos
RecoLocalCalo/HcalRecProducers

@cmsbuild, @nclopezo, @StoyanStoynev, @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.
@nclopezo you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Nov 19, 2014

Hi Phil,
my last comparisons were with respect to what we have in the release, 730pre2 to be specific (the previous iteration of method 2).

@StoyanStoynev
Copy link
Contributor

+1
For b88853a.
I retested the PR (latest commit) on top of CMSSW_7_3_X_2014-11-17-0200. My summary given for a previous commit holds overall but the MET plots Slava shows above is what I see now too (so this is slightly different). I was giving CPU timing (wf 25202) before as 0.783724 s/evt for hbheprereco now it is
0.448030 s/evt.
I have checked that the customization options work and ran on 10 events with the following results for CPU time (the significant digits just mean I am quoting what I see):
Method0: 0.060581 s/evt
Method1: 0.060961 s/evt
Method2 (default): 0.433600 s/evt
It is understood that more reduction in time should be attempted after this PR is merged.
Also (performance) differences observed or hints for differences wrt to the IB (latest pre-release) should be better scrutinized (with higher statistics samples as a first step).

@cmsbuild
Copy link
Contributor

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

@davidlange6
Copy link
Contributor

merging unless someone suggests not to soon (but I think I have understood that this PR is not yet blessed as sufficient for pre3)

@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

7 participants