Skip to content
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

Merged
merged 1 commit into from
May 28, 2024

Conversation

stahlleiton
Copy link
Contributor

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:

@cmsbuild
Copy link
Contributor

cmsbuild commented May 23, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45024/40316

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @stahlleiton for master.

It involves the following packages:

  • CondCore/PhysicsToolsPlugins (db)
  • CondCore/Utilities (db)
  • CondFormats/DataRecord (alca, db)
  • CondFormats/PhysicsToolsObjects (alca, db)
  • CondTools/DeDx (****)

The following packages do not have a category, yet:

CondTools/DeDx
Please create a PR for https://github.com/cms-sw/cms-bot/blob/master/categories_map.py to assign category

@francescobrivio, @perrotta, @cmsbuild, @saumyaphor4252, @consuegs can you please review it and eventually sign? Thanks.
@PonIlya, @tocheng, @mmusich, @yuanchao, @seemasharmafnal, @rsreds, @JanChyczynski this is something you requested to watch as well.
@antoniovilela, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

@mandrenguyen
Copy link
Contributor

Hello, since the creation of the new directory CondTools/DeDx triggers the new-package-pending flag, may I ask the experts if this is the appropriate place? I guess that's a question to alca. If that new package is ok, should Andre make the PR to cms-bot indicating alca, db as owners?

@@ -0,0 +1,18 @@
#ifndef CondFormats_PhysicsToolsObjects_DeDxCalibration_h
#define CondFormats_PhysicsToolsObjects_DeDxCalibration_h
Copy link
Contributor

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

Copy link
Contributor

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.

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;
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in f596006

@perrotta
Copy link
Contributor

Hello, since the creation of the new directory CondTools/DeDx triggers the new-package-pending flag, may I ask the experts if this is the appropriate place? I guess that's a question to alca. If that new package is ok, should Andrea make the PR to cms-bot indicating alca, db as owners?

@mandrenguyen I don't have anything against it...
And perhaps, having read your comment I could also suggest a possible CondFormats/DeDx for #45024 (comment)

Once we agree on the names I will prepare the PR to cms-bot, that's fast

@perrotta
Copy link
Contributor

And perhaps, having read your comment I could also suggest a possible CondFormats/DeDx for #45024 (comment)

Let stick on CondFormats/PhysicsToolsObjects given the comment in https://github.com/cms-sw/cmssw/pull/45024/files/8900d19fd0d7ef0df60a76bab55977b48f2ce046#r1611884639 : no objections

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45024/40322

@cmsbuild
Copy link
Contributor

Pull request #45024 was updated. @consuegs, @perrotta, @saumyaphor4252, @francescobrivio, @cmsbuild can you please check and sign again.

Comment on lines 18 to 21
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_; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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)

Copy link
Contributor Author

@stahlleiton stahlleiton May 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in f2d610d

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45024/40328

@stahlleiton
Copy link
Contributor Author

stahlleiton commented May 25, 2024

@stahlleiton @mandrenguyen , this being a new CondFormat, I think it could be worth describing and discussing it a bit in a AlCaDB meeting before merging and implementing it. I understand it is quite late to ask, but would you be available to quickly present it at the AlCaDB meeting on Monday 27th?
Just a description of the class members and the expected usage of the CondFormat, so that to be sure it is consolidated enough and no further updates are in sight (because CondFormat's are not expected being ever updated): maybe two three slides can be prepared quickly (but we will not complain if you don't manage to do, of course):

@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.

@perrotta
Copy link
Contributor

@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.

@perrotta
Copy link
Contributor

unhold

@cmsbuild
Copy link
Contributor

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)

@antoniovilela
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit c6a2524 into cms-sw:master May 28, 2024
11 checks passed
@stahlleiton stahlleiton deleted the dEdxCond_CMSSW_14_1_X branch May 28, 2024 14:48
@stahlleiton
Copy link
Contributor Author

@antoniovilela : thanks for merging.
When is the next pre-release for 14_1_X planned to be made?

@perrotta
Copy link
Contributor

@antoniovilela : thanks for merging. When is the next pre-release for 14_1_X planned to be made?

https://twiki.cern.ch/twiki/bin/view/CMS/CMSSW_14_1_0

@stahlleiton
Copy link
Contributor Author

stahlleiton commented May 28, 2024

@antoniovilela : thanks for merging. When is the next pre-release for 14_1_X planned to be made?

https://twiki.cern.ch/twiki/bin/view/CMS/CMSSW_14_1_0

ok, I see. So roughly in a month.
@perrotta : Is it possible to already start testing the new conditions? (i.e. add them to a preliminary global tag to test the other PR)

@perrotta
Copy link
Contributor

@perrotta : Is it possible to already start testing the new conditions? (i.e. add them to a preliminary global tag to test the other PR)

@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
At the moment, we have 141X queues opened for data, and for HI MC: please let us know where the new tag will be needed.

@stahlleiton
Copy link
Contributor Author

stahlleiton commented May 29, 2024

@perrotta : Is it possible to already start testing the new conditions? (i.e. add them to a preliminary global tag to test the other PR)

@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 At the moment, we have 141X queues opened for data, and for HI MC: please let us know where the new tag will be needed.

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

@perrotta
Copy link
Contributor

@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)

@stahlleiton
Copy link
Contributor Author

stahlleiton commented May 30, 2024

@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

@perrotta
Copy link
Contributor

@stahlleiton I have created two GT candidatates, one for HI_MC and one for data:

  • 141X_mcRun3_2024_realistic_HI_Candidate_2024_05_30_15_21_51 (141X_mcRun3_2024_realistic_HI_v2 + dedxCalibration tag)
  • 141X_dataRun3_Prompt_Candidate_2024_05_30_15_49_55 (141X_dataRun3_Prompt_v2 + dedxCalibration tag)

Give them a try with the other PR

@mmusich
Copy link
Contributor

mmusich commented May 30, 2024

cmsrel CMSSW_14_1_X_2024-05-29-2300
cd CMSSW_14_1_X_2024-05-29-2300/src/
cmsenv
git cms-addpkg DataFormats/TrackReco RecoTracker/Configuration RecoTracker/DeDx
wget https://patch-diff.githubusercontent.com/raw/cms-sw/cmssw/pull/45016.diff
git apply 45016.diff
git cms-checkdeps -a
scram b -j 20
runTheMatrix.py -l 180,180.1,181,181.1 --command='--conditions 141X_mcRun3_2024_realistic_HI_Candidate_2024_05_30_15_21_51'

runs fine.
I have a concern though:

$ 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 mc synchronization (and as per DB uploader policy, it won't be possible to update it further).
Is not foreseen for the data tag to be able to evolve? Shouldn't there be 2 tags: one with mc sychronization for simulation and another with prompt synchronization for data?

@stahlleiton
Copy link
Contributor Author

stahlleiton commented May 30, 2024

cmsrel CMSSW_14_1_X_2024-05-29-2300
cd CMSSW_14_1_X_2024-05-29-2300/src/
cmsenv
git cms-addpkg DataFormats/TrackReco RecoTracker/Configuration RecoTracker/DeDx
wget https://patch-diff.githubusercontent.com/raw/cms-sw/cmssw/pull/45016.diff
git apply 45016.diff
git cms-checkdeps -a
scram b -j 20
runTheMatrix.py -l 180,180.1,181,181.1 --command='--conditions 141X_mcRun3_2024_realistic_HI_Candidate_2024_05_30_15_21_51'

runs fine.
I have a concern though:

$ 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 mc synchronization (and as per DB uploader policy, it won't be possible to update it further).
Is not foreseen for the data tag to be able to evolve? Shouldn't there be 2 tags: one with mc sychronization for simulation and another with prompt synchronization for data?

Yes, the MC and data tags should eventually be different. The data one will evolve depending on the run number.

@perrotta
Copy link
Contributor

Thank you @mmusich for the checks.
The GT candidates were done to allow to allow @stahlleiton and the HI groups to test their code. For the sincronization of the final data tag we would like to understand whether it will be needed only for prompt/offline or also for express/hlt. I think it is the first one, but let me check with the consumers.
Moreover, I would like to understand how it will be included in the current workflows: as you know, as of now the 140X GTs are used also in the development release 141X via autoCond, while only the HI_MC is based on 141X: if we want to include the tag in the data GTs we must understand how to deal with it at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants