Adding MacroPixel inefficiency in Tracker Phase2Digitizer #37170
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37170/28735
|
|
A new Pull Request was created by @suchandradutta (Suchandra Dutta) for master. It involves the following packages:
@cmsbuild, @AdrianoDee, @srimanob, @civanch, @mdhildreth can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild, please test |
| bool PSPDigitizerAlgorithm::isInBiasRailRegion(const PSimHit& hit) const { | ||
| static const float implant = 0.1467; // Implant length (1.467 mm) | ||
| static const float bRail = 0.00375; // Bias Rail region which causes inefficiency (37.5micron) | ||
| float block_len = 16 * implant + 15.5 * bRail; |
There was a problem hiding this comment.
This could be static const too (or even better constexpr, this applies to the two above too).
There was a problem hiding this comment.
for future reference, maybe you can also put a comment on what are 16 and 15.5?
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f3a7af/22932/summary.html Comparison SummarySummary:
|
cmsbuild
added
code-checks-pending
tests-pending
and removed
tests-approved
code-checks-approved
labels
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37170/28754
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
|
hi @suchandradutta, can you apply the code-format as requested by the bot above: #37170 (comment):
|
|
test parameters:
|
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f3a7af/22964/summary.html Comparison Summary@slava77 comparisons for the following workflows were not done due to missing matrix map:
Summary:
|
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f3a7af/22991/summary.html Comparison Summary@slava77 comparisons for the following workflows were not done due to missing matrix map:
Summary:
|
|
@cms-sw/simulation-l2 @cms-sw/upgrade-l2 This is one of the few PRs pending on the boundary of pre6, please have a look at your earliest convenient time. Thanks! |
|
+Upgrade |
|
+1 |
cmsbuild
added
fully-signed
simulation-approved
and removed
simulation-pending
pending-signatures
labels
|
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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
|
|
||
| private: | ||
| edm::ESGetToken<SiPhase2OuterTrackerLorentzAngle, SiPhase2OuterTrackerLorentzAngleSimRcd> siPhase2OTLorentzAngleToken_; | ||
| const edm::ESGetToken<TrackerGeometry, TrackerDigiGeometryRecord> geomToken_; | ||
| bool addBiasRailInefficiency_{false}; |
There was a problem hiding this comment.
I may be missing something but If you set it here to false and then you ask for the parameter to be in the config (here) this false will be always overwritten by the input parameter, no? If the intention was to have a default value for this (without having to specify it in the python configuration) you would need to do something like:
SPDigitizerAlgorithm::PSPDigitizerAlgorithm(const edm::ParameterSet& conf, edm::ConsumesCollector iC)
: Phase2TrackerDigitizerAlgorithm(conf.getParameter<ParameterSet>("AlgorithmCommon"),
conf.getParameter<ParameterSet>("PSPDigitizerAlgorithm"),
iC),
geomToken_(iC.esConsumes()) {
if (conf.getParameter<ParameterSet>("PSPDigitizerAlgorithm").exists("AddBiasRailInefficiency"))
{
addBiasRailInefficiency_ = conf.getParameter<ParameterSet>("PSPDigitizerAlgorithm").getParameter<bool>("AddBiasRailInefficiency");
}(note that this compiles but may be prone to errors.)
To have it set to false if the parameter is not in the config.
There was a problem hiding this comment.
[ P.s. I thought to have already commented here but actually I had not pushed "Submit the review". ]
There was a problem hiding this comment.
@AdrianoDee is correct - there is no sense in this construction. This should be done in utilities and not for each specific parameters. For me it is much more clear when there a python fragment responsible for parameters of this class with default values of all parameters.
There was a problem hiding this comment.
This may be fixed in the new PR
There was a problem hiding this comment.
This may be fixed in the new PR
Thanks @civanch, yes I agree (@suchandradutta).
There was a problem hiding this comment.
Isn't exists actively discouraged? #36261
There was a problem hiding this comment.
Indeed you are right. Still I would think that false is useless but I don't know how to parse a default parameter in this context (usually I go through a fillDescriptions but I'm not sure about how it may be implemented here).
There was a problem hiding this comment.
I guess one would need a fillPSetDecription in the mother class that is then percolated to the individual plugins making use of the algorithm. I think this would need some larger refactoring of code (what about opening an issue?)
PR description:
Adding macropixel inefficiency due to bias rail in the Phase2Digitizer framework in PSPDigitizerAlgorithm
PR validation:
The validation is done using
runTheMatrix.py --what upgrade -ne -l 39010.0
The description and the plots are attached below
Phase2Digitizer_BiasRailInEfficiency_07032022.pdf
if this PR is a backport please specify the original PR and why you need to backport that PR: Not backported
@emiglior @mmusich @tsusa