moved hardcoded correction boundary values to configuration file in EcalLaserCondTools #30010
Conversation
|
The code-checks are being triggered in jenkins. |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30010/15701
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30010/15702
|
|
A new Pull Request was created by @grasph (Philippe Gras) for master. It involves the following packages: CalibCalorimetry/EcalTrivialCondModules @cmsbuild, @pohsun, @christopheralanwest, @tocheng, @tlampen can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
The tests are being triggered in jenkins. |
|
+1 |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
+1 |
cmsbuild
added
alca-approved
fully-signed
and removed
alca-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. @silviodonato, @dpiparo (and backports should be raised in the release meeting by the corresponding L2) |
There was a problem hiding this comment.
Looking at "6 – Packaging Rules" https://cms-sw.github.io/cms_coding_rules.html it seems EcalLaserCondTools should go in a single .cc file in /plugins directory.
| toTime_(ps.getParameter<int>("toTime")), | ||
| minP_(ps.getParameter<double>("transparencyMin")), | ||
| maxP_(ps.getParameter<double>("transparencyMax")) { |
There was a problem hiding this comment.
Adding new parameters means that current py files without the new parameters will crash in future CMSSW release and that the newer py files with the new parameters will crash in the older CMSSW release. I would suggest to introduce the fillDescriptions to set default values in order to preserve the compatibility between the releases (https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideConfigurationValidationAndHelp#An_Example)
There was a problem hiding this comment.
I don't understand. I expect that by "crash" you meant an exception, not a core dump.
The parameters are tracked parameters, so an exception will already be raised if they are missing in the configuration file.
There was a problem hiding this comment.
In addition this "fillDescription" is a source of inconsistency in the code. The new of the parameters will appear twice with the risk to update at one place and forget in another place or using a different spelling. I have missed the rationally of this new method.
|
The main comment is #30010 (comment). You can consider the other comments as "nice to have" features going beyond the purpose of this PR. |
silviodonato
changed the title
|
+1 |
Two parameters (correction boudaries) were hardcoded in the module EcalLaserCondTools. This PR makes them configurable thanks to the work of @fcouderc . Application of correction boundaries has also been simplified.
The code is not used in the reco and simulation workflows.