New fixes to HF FG bit #12130
New fixes to HF FG bit #12130
Conversation
|
A new Pull Request was created by @mnorthup6690 for CMSSW_7_5_X. New fixes to HF FG bit It involves the following packages: SimCalorimetry/HcalTrigPrimAlgos @cmsbuild, @mulhearn can you please review it and eventually sign? Thanks. |
|
This needs to be merged for HI data taking. We have validated that it matches data, see below. https://twiki.cern.ch/twiki/pub/Main/MBEmulation2015/MinBiasEmulation_Comparison_2015-11-05.pdf |
|
please test |
|
+1 |
|
The tests are being triggered in jenkins. |
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_7_5_X IBs after it passes the integration tests and once validation in the development release cycle CMSSW_7_6_X is complete. This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @smuzaffar |
cmsbuild
added
l1-approved
fully-signed
tests-started
and removed
l1-pending
pending-signatures
tests-pending
labels
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_7_5_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_7_6_X is complete. This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @smuzaffar |
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_7_5_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_7_6_X is complete. This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @smuzaffar |
| else if (phi == 3 || phi == 5) | ||
| { | ||
| GCTphi = 1; | ||
| } |
There was a problem hiding this comment.
I think this could better be
if (phi==71) GCTphi=0;
else GCTphi=(phi+1)/4
|
@mnorthup6690 - and there are several other cases where the if ( phi... ) statements can be made much simpler. Could you have a look. Thanks! |
|
Not looking at the exact relations between the values in these "else-if", I've suggested "lookup" map in a sibling PR #12129 Now I see David has spotted there are very simple functions which can possibly replace all these "trees"... |
|
@davidlange6 @abdoulline I propose we leave this is implemented here in 75X... it has been tested after all, and is urgently needed... and pursue the better implemented version in the 80X branch. |
|
Improving 80X-only version sounds reasonable |
|
@mulhearn -is that to say that testing the change by looking at dqm histograms is insufficient for knowing if the relatively straightfoward changes are ok or not? [these promised future changes in CMS tend not to happen on average:)] |
|
@davidlange6: Indeed, I was suggesting we don't merge the request as in the 80X branch at all, only for the 75X. We only accept cleaned up version in 80X. |
|
It seems 80X version is already updated... |
|
@davidlange6 You can have a look at discussion here #12287. The cleaned up version is available for 80X already, but we prefer to keep this version in 75X (urgently needed) as it has been more thoroughly tested. In this case, I think we can have our cake and eat it too. |
|
Thank you! |
Also merging with 76X #12129.