Remove hardcoded SOI from HF digis validation #18498
Conversation
…query digi.presamples() to determine the proper behavior.
|
A new Pull Request was created by @kencall for master. It involves the following packages: Validation/HcalDigis @cmsbuild, @dmitrijus, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
| double fBin67 = tool[3] + tool[4] | ||
| - calibrations.pedestal((*digiItr)[3].capid()) | ||
| - calibrations.pedestal((*digiItr)[4].capid()); | ||
| int soi = (tool.presamples() == 1 ? 1 : 2); |
There was a problem hiding this comment.
Is there a reason this can't just be:
int soi = tool.presamples();
?
There was a problem hiding this comment.
No reason, I had coded it this way because it was a choice between two different behaviors. I then saw this alternative, but decided to leave it as is. If you hold off triggering tests, I'll change the code.
|
please test |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
So Ken, this is a "minimum minimorum" (for HF only, while HBHE/HO parts stays unchanged/hardcoded). |
|
Hi Salavat,
That is a good point. If we can trust that the SOI is always indexed directly after all of the presamples, and that the number of presamples is correctly provided by the presamples() method (which is a good assumption), then there should be no problem future proofing HB/HE/HO using the same logic.
This would actually simplify the code, since HF would no longer be a special case.
Regards,
Ken Call
From: Salavat Abdullin [mailto:notifications@github.com]
Sent: Thursday, April 27, 2017 4:00 PM
To: cms-sw/cmssw <cmssw@noreply.github.com>
Cc: Call, Ken <Ken_Call@baylor.edu>; Mention <mention@noreply.github.com>
Subject: Re: [cms-sw/cmssw] Remove hardcoded SOI from HF digis validation (#18498)
So Ken, this is a "minimum minimorum" (for HF only, while HBHE/HO parts stays unchanged/hardcoded).
There is always a chance that this year a similar change may happen to HB (well, alternatively it might be "just" ZS rise).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#18498 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AJHhzdYc3wDEDQZezpyhnkzCZw-Gngb1ks5r0QHVgaJpZM4NKiwi>.
|
|
Hi Ken, we do (trust), it's by construction (both in firmware and in MC).
…On Thu, 27 Apr 2017, kencall wrote:
Hi Salavat,
That is a good point. If we can trust that the SOI is always indexed directly after all of
the presamples, and that the number of presamples is correctly provided by the presamples()
method (which is a good assumption), then there should be no problem future proofing
HB/HE/HO using the same logic.
This would actually simplify the code, since HF would no longer be a special case.
Regards,
Ken Call
From: Salavat Abdullin ***@***.***
Sent: Thursday, April 27, 2017 4:00 PM
To: cms-sw/cmssw ***@***.***>
Cc: Call, Ken ***@***.***>; Mention ***@***.***>
Subject: Re: [cms-sw/cmssw] Remove hardcoded SOI from HF digis validation (#18498)
So Ken, this is a "minimum minimorum" (for HF only, while HBHE/HO parts stays
unchanged/hardcoded).
There is always a chance that this year a similar change may happen to HB (well,
alternatively it might be "just" ZS rise).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on
GitHub<#18498 (comment)>, or mute thethread<https://github.com/notifications/unsubscribe-auth/AJHhzdYc3wDEDQZezpyhnkzCZw-Gngb1k
s5r0QHVgaJpZM4NKiwi>.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the
thread.[AEx02o7_fx5R-hQ9XY2iETTu8mwFzVaVks5r0QN7gaJpZM4NKiwi.gif]
|
|
Hi Salavat and all,
I’ll implement the general solution for all the subdetectors.
Regards,
Ken Call
From: Salavat Abdullin [mailto:notifications@github.com]
Sent: Friday, April 28, 2017 12:21 AM
To: cms-sw/cmssw <cmssw@noreply.github.com>
Cc: Call, Ken <Ken_Call@baylor.edu>; Mention <mention@noreply.github.com>
Subject: Re: [cms-sw/cmssw] Remove hardcoded SOI from HF digis validation (#18498)
Hi Ken, we do (trust), it's by construction (both in firmware and in MC).
On Thu, 27 Apr 2017, kencall wrote:
Hi Salavat,
That is a good point. If we can trust that the SOI is always indexed directly after all of
the presamples, and that the number of presamples is correctly provided by the presamples()
method (which is a good assumption), then there should be no problem future proofing
HB/HE/HO using the same logic.
This would actually simplify the code, since HF would no longer be a special case.
Regards,
Ken Call
From: Salavat Abdullin ***@***.***
Sent: Thursday, April 27, 2017 4:00 PM
To: cms-sw/cmssw ***@***.***>
Cc: Call, Ken ***@***.***>; Mention ***@***.***>
Subject: Re: [cms-sw/cmssw] Remove hardcoded SOI from HF digis validation (#18498)
So Ken, this is a "minimum minimorum" (for HF only, while HBHE/HO parts stays
unchanged/hardcoded).
There is always a chance that this year a similar change may happen to HB (well,
alternatively it might be "just" ZS rise).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on
GitHub<#18498 (comment)>, or mute thethread<https://github.com/notifications/unsubscribe-auth/AJHhzdYc3wDEDQZezpyhnkzCZw-Gngb1k
s5r0QHVgaJpZM4NKiwi>.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the
thread.[AEx02o7_fx5R-hQ9XY2iETTu8mwFzVaVks5r0QN7gaJpZM4NKiwi.gif]
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#18498 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AJHhzd135Peus3R7dyl-lWxr4lgAtu9Nks5r0XcugaJpZM4NKiwi>.
|
…f pre/post samples can be changed and will not conflict with hard coded values
|
Pull request #18498 was updated. @cmsbuild, @dmitrijus, @vanbesien, @davidlange6 can you please check and sign again. |
|
I've heeded Salavat's suggestion and removed the harded coded SOI from HB/HE/HO. The module can now handle dynamically the number of presamples. |
|
+1 |
|
The tests are being triggered in jenkins. |
cmsbuild
added
dqm-approved
fully-signed
and removed
dqm-pending
pending-signatures
labels
|
This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
+1 |
Validation histograms for HF digis will now query digi.presamples() to determine the SOI, previously it was hardcoded. Previously HF had 4 TS, this was changed in data to 3TS, and PR #18372 changed it in MC as well.