-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add SiStripApvSimulationParameters database object and record #27941
Add SiStripApvSimulationParameters database object and record #27941
Conversation
including an ESSource, and EDAnalyzer to add to the database
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27941/11786
|
A new Pull Request was created by @pieterdavid (Pieter David) for master. It involves the following packages: CalibTracker/SiStripESProducers @christopheralanwest, @tocheng, @cmsbuild, @franzoni, @tlampen, @ggovi, @pohsun can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test |
The tests are being triggered in jenkins. |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
@pieterdavid Should I re-run the tests including PR #27823 as well? |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
+1 |
+1 |
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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
@@ -71,7 +71,7 @@ namespace PhysicsTools { | |||
binULimitsX(binULimitsX), | |||
binULimitsY(binULimitsY), | |||
binULimitsZ(binULimitsZ), | |||
binValues((binULimitsY.size() + 1) * strideX * strideY), | |||
binValues((binULimitsZ.size() + 1) * strideX * strideY), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pieterdavid @ggovi for my understanding, this looks a bug fix, is it backward compatible in term of CondFormat? It is not changing neither the structure nor the interface of the format itself, so I would say it is...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed nobody used this constructor before, because if used to create a histogram with more Z than Y-bins, accessing a bin beyond the array would throw an exception (as long as this is not the constructor used by the deserialisation code it should be compatible).
+1 |
PR description:
This PR adds a new object and record to hold the APV simulation baseline distributions, as discussed in #27823 .
It also adds an ESSource to create it from the existing ascii files, and an EDAnalyzer to add it to the database.
Thanks @mmusich and @EmyrClement for the help and feedback!
PR validation:
Compiles, unit tests (serialization) run, created an sqlite file with a payload, adapted the digitizer to use this database (or the product directly from the ESSource in the same job). Inspected the xml of the payload and tested histogram sampling code in a standalone example.