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

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

Merged
merged 4 commits into from Jun 29, 2016

Conversation

igv4321
Copy link
Contributor

@igv4321 igv4321 commented Jun 23, 2016

A trivial change, in the anticipation of Phase 1 algorithm development.
Not expected to affect anything.

@cmsbuild
Copy link
Contributor

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

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.
@slava77, @Degano, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

@slava77
Copy link
Contributor

slava77 commented Jun 23, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 23, 2016

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

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

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

@slava77
Copy link
Contributor

slava77 commented Jun 26, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 26, 2016

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

float timeshift_ns_hbheho(float wpksamp);

/// Special energy correction for some HB- cells
float hbminus_special_ecorr(int ieta, int iphi, double energy, int runnum);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since there are two methods declared here now, the timeshift_ns_hbheho.h file name is no longer intuitive.
Please rename to something more comprising.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea what is intuitive for you. It is intuitive enough for me.
If you want it renamed, suggest a name.

On 06/26/2016 11:06 AM, Slava Krutelyov wrote:

In RecoLocalCalo/HcalRecAlgos/interface/timeshift_ns_hbheho.h #14957 (comment):

@@ -0,0 +1,14 @@
+#ifndef RecoLocalCalo_HcalRecAlgos_timeshift_ns_hbheho_h_
+#define RecoLocalCalo_HcalRecAlgos_timeshift_ns_hbheho_h_
+
+///Timeshift correction for HPDs based on the position of the peak ADC measurement.
+/// Allows for an accurate determination of the relative phase of the pulse shape from
+/// the HPD. Calculated based on a weighted sum of the -1,0,+1 samples relative to the peak
+/// as follows: wpksamp = (0_sample[0] + 1_sample[1] + 2*sample[2]) / (sample[0] + sample[1] + sample[2])
+/// where sample[1] is the maximum ADC sample value.
+float timeshift_ns_hbheho(float wpksamp);
+
+/// Special energy correction for some HB- cells
+float hbminus_special_ecorr(int ieta, int iphi, double energy, int runnum);

since there are two methods declared here now, the timeshift_ns_hbheho.h file name is no longer intuitive.
Please rename to something more comprising.


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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a file named timeshift containing energy correction is not intuitive.
CorrectionFunctions.h may be well covering what's here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine. Renamed into HcalCorrectionFunctions.h

On 06/26/2016 12:38 PM, Slava Krutelyov wrote:

In RecoLocalCalo/HcalRecAlgos/interface/timeshift_ns_hbheho.h #14957 (comment):

@@ -0,0 +1,14 @@
+#ifndef RecoLocalCalo_HcalRecAlgos_timeshift_ns_hbheho_h_
+#define RecoLocalCalo_HcalRecAlgos_timeshift_ns_hbheho_h_
+
+///Timeshift correction for HPDs based on the position of the peak ADC measurement.
+/// Allows for an accurate determination of the relative phase of the pulse shape from
+/// the HPD. Calculated based on a weighted sum of the -1,0,+1 samples relative to the peak
+/// as follows: wpksamp = (0_sample[0] + 1_sample[1] + 2*sample[2]) / (sample[0] + sample[1] + sample[2])
+/// where sample[1] is the maximum ADC sample value.
+float timeshift_ns_hbheho(float wpksamp);
+
+/// Special energy correction for some HB- cells
+float hbminus_special_ecorr(int ieta, int iphi, double energy, int runnum);

a file named timeshift containing energy correction is not intuitive.
CorrectionFunctions.h may be well covering what's here.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub https://github.com/cms-sw/cmssw/pull/14957/files/0f0937650a7012d4c9a9a387e9ebb0997eb4842c#r68506304, or mute the thread
https://github.com/notifications/unsubscribe/AFS4-QiZlp5TynSgw4i4oQb5CLYCC_g-ks5qPrj-gaJpZM4I8hdG.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

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

@slava77
Copy link
Contributor

slava77 commented Jun 26, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 26, 2016

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

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Jun 27, 2016

+1

for #14957 a3397a0

  • technical change, as described
  • jenkins tests pass and comparisons with baseline show no differences

@cmsbuild
Copy link
Contributor

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

@igv4321
Copy link
Contributor Author

igv4321 commented Jun 28, 2016

@davidlange6 Please merge, there is more code in the pipeline depending on this change.
Ditto for PR #14920 .

@davidlange6
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 05331de into cms-sw:CMSSW_8_1_X Jun 29, 2016
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

4 participants