-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 new dEdx calibration condition format #45024
Conversation
cms-bot internal usage |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45024/40316
|
A new Pull Request was created by @stahlleiton for master. It involves the following packages:
The following packages do not have a category, yet: CondTools/DeDx @francescobrivio, @perrotta, @cmsbuild, @saumyaphor4252, @consuegs can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
Hello, since the creation of the new directory |
@@ -0,0 +1,18 @@ | |||
#ifndef CondFormats_PhysicsToolsObjects_DeDxCalibration_h | |||
#define CondFormats_PhysicsToolsObjects_DeDxCalibration_h |
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 wonder if PhysicsToolsObjects
is the correct place where to put this DeDxCalibration class.
Maybe it is, and indeed, I'm not seeing anything better than this within the current packages in CondFormats
... But perhaps a new CondFormats/Tracker
could be a more appropriate one?
I'm open to suggestions
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.
CondFormats/Tracker could be a more appropriate one?
I disagree. This object has nothing to do with Tracker (DPG) calibrations, but rather with physics applications. Please be reminded that in CondFormats/PhysicsToolsObjects
there are already objects of the type PhysicsTools::Calibration::HistogramD2D
and PhysicsTools::Calibration::HistogramD3D
that are already associated with dE/dx calibration related records.
cmssw/CondCore/PhysicsToolsPlugins/src/plugin.cc
Lines 32 to 37 in 356b35d
REGISTER_PLUGIN(SiStripDeDxMipRcd, PhysicsTools::Calibration::HistogramD2D); | |
REGISTER_PLUGIN(SiStripDeDxMip_3D_Rcd, PhysicsTools::Calibration::HistogramD3D); | |
REGISTER_PLUGIN_NO_SERIAL(SiStripDeDxProton_3D_Rcd, PhysicsTools::Calibration::HistogramD3D); | |
REGISTER_PLUGIN_NO_SERIAL(SiStripDeDxPion_3D_Rcd, PhysicsTools::Calibration::HistogramD3D); | |
REGISTER_PLUGIN_NO_SERIAL(SiStripDeDxKaon_3D_Rcd, PhysicsTools::Calibration::HistogramD3D); | |
REGISTER_PLUGIN_NO_SERIAL(SiStripDeDxElectron_3D_Rcd, PhysicsTools::Calibration::HistogramD3D); |
public: | ||
DeDxCalibration(); | ||
virtual ~DeDxCalibration() {} | ||
std::vector<double> thr; |
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.
These class members should better be made private
, and then accessed through public
getter's and setter's
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.
Addressed in f596006
@mandrenguyen I don't have anything against it... Once we agree on the names I will prepare the PR to cms-bot, that's fast |
Let stick on CondFormats/PhysicsToolsObjects given the comment in https://github.com/cms-sw/cmssw/pull/45024/files/8900d19fd0d7ef0df60a76bab55977b48f2ce046#r1611884639 : no objections |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45024/40322
|
Pull request #45024 was updated. @consuegs, @perrotta, @saumyaphor4252, @francescobrivio, @cmsbuild can you please check and sign again. |
std::vector<double> getThr() const { return thr_; } | ||
std::vector<double> getAlpha() const { return alpha_; } | ||
std::vector<double> getSigma() const { return sigma_; } | ||
std::map<ChipId, float> getGain() const { return gain_; } |
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.
std::vector<double> getThr() const { return thr_; } | |
std::vector<double> getAlpha() const { return alpha_; } | |
std::vector<double> getSigma() const { return sigma_; } | |
std::map<ChipId, float> getGain() const { return gain_; } | |
std::vector<double> thr() const { return thr_; } | |
std::vector<double> alpha() const { return alpha_; } | |
std::vector<double> sigma() const { return sigma_; } | |
std::map<ChipId, float> gain() const { return gain_; } |
(To comply with the rule nr 11 in https://cms-sw.github.io/cms_coding_rules.html#2--naming-rules-1)
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.
Addressed in f2d610d
f596006
to
64e7dd5
Compare
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45024/40328
|
@perrotta : I will be travelling from Japan and will arrive in Geneva on Monday. So in principle I could present at the AlcaDB meeting as long as there are no delays in my flights. |
Thank you @stahlleiton for your availability! I have added you in agenda. But, of course: don't feel obliged even if you are too much tired after such a long trip. In any case we don't expect a long discussion. |
unhold |
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. @rappoccio, @antoniovilela, @sextonkennedy (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
@antoniovilela : thanks for merging. |
|
ok, I see. So roughly in a month. |
@stahlleiton please provide the tag, or the db file with the new tag, and we'll take care of adding it to a 141X GT |
I copied the db file in /afs/cern.ch/user/a/anstahll/work/public/ForDeDx/dedxCalibration.db . We could add it to both the MC and data GTs, and then run the relvals using both data and MC |
@stahlleiton I see you (or someone else) created already a tag in confDB: https://cms-conddb.cern.ch/cmsDbBrowser/list/Prod/tags/DeDxCalibration_HI_2024_v1_Test Do you want to stick on it? (I would have avoided the string "_Test", but I am not particularly fond on it) |
I did not create it (actually I don't know how to do it). It was made by Saumya to quickly test things. But yeah, I would rather prefer not to have the "_Test" in the tag name for the corrections that will be used in 2024 data taking. Let me know once you have the global tag ready, and I will update the other PR so that it can be used by the relvals |
@stahlleiton I have created two GT candidatates, one for HI_MC and one for data:
Give them a try with the other PR |
runs fine. $ conddb list 141X_mcRun3_2024_realistic_HI_Candidate_2024_05_30_15_21_51 | grep DeDxCalibration
[2024-05-30 20:02:37,584] INFO: Connecting to pro [frontier://PromptProd/cms_conditions]
DeDxCalibrationRcd - DeDxCalibration_HI_2024_v1
$ conddb list 141X_dataRun3_Prompt_Candidate_2024_05_30_15_49_55 | grep DeDxCalibration
[2024-05-30 20:03:04,662] INFO: Connecting to pro [frontier://PromptProd/cms_conditions]
DeDxCalibrationRcd - DeDxCalibration_HI_2024_v1 so both data and MC GlobalTag candidates have the same tag. This means that this tag will have to have |
Yes, the MC and data tags should eventually be different. The data one will evolve depending on the run number. |
Thank you @mmusich for the checks. |
PR description:
This PR adds a new dEdx condition format for dEdx gain calibrations meant for the 2024 PbPb UPC reco. It includes a new condition format DeDxCalibration to store the pixel/strip chip-level gain calibrations and strip detector properties (threshold t, coupling α and standard deviation σ of the Gaussian noise derived from data) and a producer DeDxCalibrationDbCreator to derive the calibration database files.
The details of the dEdx calibration for 2023 PbPb UPC are documented in results and in the CADI HIN-12-016.
This PR is needed for #45016
@mandrenguyen @sikler
PR validation:
If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for: