HFNose: implement aging #31981
HFNose: implement aging #31981
Conversation
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31981/19456
|
|
A new Pull Request was created by @mariadalfonso for master. It involves the following packages: SLHCUpgradeSimulations/Configuration @cmsbuild, @civanch, @kpedro88, @mdhildreth can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
| @@ -66,13 +65,13 @@ class HGCalSiNoiseMap : public HGCalRadiationMap { | |||
| @short returns the charge collection efficiency and noise | |||
| if gain range is set to auto, it will find the most appropriate gain to put the mip peak close to 10 ADC counts | |||
| */ | |||
| const SiCellOpCharacteristicsCore getSiCellOpCharacteristicsCore(const HGCSiliconDetId &did, | |||
| const SiCellOpCharacteristicsCore getSiCellOpCharacteristicsCore(const unsigned cell_raw_Id, | |||
| GainRange_t gain, | |||
There was a problem hiding this comment.
I do not particularly like this approach. In my opinion, this goes against the strongly typed nature of C++ and reminds me of the void *, used and abused in the old C-days.
This is also introducing unnecessary branching conditions in the code, that will end up wasting CPU cycles. They are unnecessary since the branch is fully known and determined by the correct type of DetId.
I'd therefore suggest a factorization of the part of the code that could stay untouched and in common between the 2 detectors, and use a template+specialization to handle the cases driven by the type.
There was a problem hiding this comment.
thanks for the feedback, I can template this class
give me a couple of days
mariadalfonso
force-pushed
the
from-CMSSW_11_2_0_pre7_AGING
branch
from
fd3ae1e
to
d7d3ece
Compare
|
The code-checks are being triggered in jenkins. |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31981/19504
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
mariadalfonso
force-pushed
the
from-CMSSW_11_2_0_pre7_AGING
branch
from
d7d3ece
to
21c0c1d
Compare
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31981/19505
|
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31981/19857
|
|
Pull request #31981 was updated. @cmsbuild, @civanch, @kpedro88, @mdhildreth can you please check and sign again. |
|
please test |
|
The tests are being triggered in jenkins.
|
|
+1 |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
+1 |
|
+upgrade |
|
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 will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
cmsbuild
added
fully-signed
upgrade-approved
and removed
pending-signatures
upgrade-pending
labels
|
+1 |
Input from BRIL feb2020
https://indico.cern.ch/event/875501/contributions/3765913/attachments/1994019/3326168/HFNose5.pdf
Various validation plots obtained with the SW in this PR are here
http://dalfonso.web.cern.ch/dalfonso/HGCAL/AGING/
No changes expected in the HGCAL scenario 23234.103
@franzoni @pfs