New high granularity pixel alignment for the PCL #38449

dmeuser · 2022-06-21T11:33:20Z

PR description:

This is the last PR towards using a higher granularity (HG) for the pixel alignment in the PCL, following #38195, #38286, #38273, #38367. It is planned to run the HG alignment in parallel to the LG alignment, with only the LG alignment updating the conditions. More details can also be found in [1].

In detail the following things are changed/added in this PR:

Adapt MillePedeFileReader and MillePedeDQMModule to read and store the high granularity alignment
Read and use the new thresholds in MillePedeDQMModule and MillePedeAlignmentAlgorithm
Store/read the pede results for the HG alignment in/from a separate folder, to avoid overwriting the LG result in the harvesting step.
Two new methods in the new HG threshold object, to get and store the float maps
Add workflows for the production and the harvesting required for the HG alignment in CommonAlignmentProducer
Add the new workflows to the PCL configuration
Add the LG and HG alignment to the 1001.2 workflows, which can be used with the runTheMatrix.py command
Add a relval customizer in CommonAlignmentProducer to override the 20 LS veto and required entries per structure in the HG aligment

PR validation:

The PR can be tested using the adapted relval workflow with runTheMatrix.py -l 1001.2.

cc:
@mmusich, @connorpa, @antoniovagnerini, @consuegs

[1] https://indico.cern.ch/event/1167313/contributions/4906905/attachments/2461217/4219804/AlCaDB_Meeting_13_06_22.pdf

cmsbuild · 2022-06-21T11:41:53Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38449/30656

This PR adds an extra 124KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File Configuration/PyReleaseValidation/python/relval_steps.py modified in PR(s): Rawprime workflow with approximated siStrip clusters for heavy ions #38423, Update tau embed wf #38266

cmsbuild · 2022-06-21T11:42:23Z

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

It involves the following packages:

Alignment/CommonAlignmentProducer (alca)
Alignment/MillePedeAlignmentAlgorithm (alca)
CondFormats/PCLConfig (db, alca)
Configuration/AlCa (alca)
Configuration/EventContent (operations)
Configuration/PyReleaseValidation (pdmv, upgrade)
Configuration/StandardSequences (operations)

@perrotta, @malbouis, @yuanchao, @jordan-martins, @bbilin, @tvami, @cmsbuild, @AdrianoDee, @srimanob, @kskovpen, @ggovi, @qliphy, @francescobrivio, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks.
@felicepantaleo, @kpedro88, @Martin-Grunewald, @tlampen, @trtomei, @ChrisMisan, @mmusich, @slomeo, @pakhotin, @makortel, @JanFSchulte, @dgulhan, @missirol, @seemasharmafnal, @beaucero, @GiacomoSguazzoni, @rovere, @VinInn, @tocheng, @ebrondol, @mtosi, @fabiocos, @adewit, @lecriste this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

Alignment/CommonAlignmentProducer/python/ALCARECOPromptCalibProdSiPixelAliHG_cff.py

Alignment/CommonAlignmentProducer/python/customizeLSNumberFilterForRelVals.py

Alignment/CommonAlignmentProducer/python/ALCARECOPromptCalibProdSiPixelAliHG_Output_cff.py

Alignment/CommonAlignmentProducer/python/ALCARECOPromptCalibProdSiPixelAliHG_cff.py

Alignment/CommonAlignmentProducer/python/AlcaSiPixelAliHarvesterHG_cff.py

mmusich · 2022-06-21T12:26:58Z

Alignment/MillePedeAlignmentAlgorithm/plugins/MillePedeDQMModule.cc

+  } else {
+    booker.setCurrentFolder("AlCaReco/SiPixelAliHG/");
+
+    layerVec = {{"Layer1", 12},


maybe all of this can be taken from ES instead of hardcoding?

cmssw/Geometry/TrackerGeometryBuilder/interface/PixelTopologyMap.h

Lines 32 to 35 in a010308

inline const unsigned getPXBLadders(unsigned int lay) const { return m_pxbMap.at(lay).first; }

inline const unsigned getPXBModules(unsigned int lay) const { return m_pxbMap.at(lay).second; }

inline const unsigned getPXFBlades(int disk) const { return m_pxfMap.at(std::abs(disk)).first; }

inline const unsigned getPXFModules(int disk) const { return m_pxfMap.at(std::abs(disk)).second; }

Sure, however from getPXFModules I get only 1 module for each disk. Is this expected? I could instead use getPXFBlades and multiply the result by 2.

unfortunately as far as I can see from here it is:

Subdetector 2 (DetId::subDetId() == PixelSubdetector::PixelEndcap): Phase1 Pixel Forward

Name start bit hex mask bit size TrackerTopology method Notes

subdetector part 23 0x3 2 pxfSide(id) or side(id) 1=FPIX- 2=FPIX+

Blade 12 0x3F 6 pxfBlade(id) increasing phi and r: first inner ring blades and the outer ring blades

Panel 10 0x3 2 pxfPanel(id) 1=forward 2=backward

Module 2 0xFF 8 pxfModule(id) always = 1

though admittedly that's not the way I thought it was supposed to work when I wrote the code.
I guess you can go ahead with the blade implementation while I pick up the slack...

mmusich · 2022-06-21T12:29:00Z

Alignment/MillePedeAlignmentAlgorithm/src/MillePedeFileReader.cc

+    case 15:
+      return (tns.tpe().halfCylinderNumber(id) == 1)
+                 ? (tns.tpe().bladeNumber(id) * 2 - (tns.tpe().panelNumber(id) % 2)) + 148
+                 : (tns.tpe().bladeNumber(id) * 2 - (tns.tpe().panelNumber(id) % 2)) + 148 + 56;


can we reduce here the amount of harcoded constants?

I'm struggling a bit with this one. Probably it would be nice also to use the PixelTopologyMap, but if I'm not mistaken I can not take it from ES in the MillePedeFileReader, right?

In principle you can make an object of the PixelTopologyMap class a data member of MillePedeFileReader that gets initialized elsewhere and then access here its methods, though if it slows you down too much, we can think on how to do it in a follow-up (of course if the other reviewers don't insist).

I already thought about this, but the MillePedeFileReader is not only used in MillePedeDQMModule but also in MillePedeAlignmentAlgorithm, so there would be a few more changes when using the PixelTopologyMap. I am still not sure if it gets much better with this one.
So maybe we put this aside for now and come back to this later (hopefully with a better idea)

but also in MillePedeAlignmentAlgorithm

of course it would need to have all the callers changed, but presumably all of them have access to the ES.

I decided to try it using the PixelTopologyMap, lets see how this turns out :)

I came up with a solution, with almost no constants. Hope this is better now: 6e9c819

francescobrivio · 2022-06-21T12:36:35Z

Configuration/StandardSequences/python/AlCaHarvesting_cff.py

+ALCAHARVESTSiPixelAliHG_metadata = cms.PSet(record = cms.untracked.string('TrackerAlignmentRcd'))
+
+ALCAHARVESTSiPixelAliHG_dbOutput = cms.PSet(record = cms.string('TrackerAlignmentRcd'),
+                                          tag = cms.string('SiPixelAli_pclHG'),
+                                          timetype   = cms.untracked.string('runnumber')
+                                          )


I think using TrackerAlignmentRcd here might conflict with the key used in the metadata for the current SiPixel Alignment. I think it should be renamed.

Also, related to this, we should update the DropboxMetadata code (and the tags) in https://github.com/cms-sw/cmssw/blob/master/CondFormats/Common/test/ProduceDropBoxMetadata.py. @dmeuser do you want to include the changes in this PR or do you want us to do it in a different PR?

related to this, please update also the unit test at https://github.com/cms-sw/cmssw/blob/master/Calibration/TkAlCaRecoProducers/test/testPCLAlCaHarvesting.py

do you want to include the changes in this PR or do you want us to do it in a different PR?

As you prefer, if you want me to take care of it I would have the following question:
Do we need a new record? Or is renaming the one used in AlCaHarvesting_cff.py enough?
And for the changes in DropboxMetadata and in the unit test, can I just duplicate the LG ones and put the corresponding HG parts there?

taking the liberty of answering, since no-one from AlCa feels like commenting:

Do we need a new record?

No.

Or is renaming the one used in AlCaHarvesting_cff.py enough?

yes.

And for the changes in DropboxMetadata and in the unit test, can I just duplicate the LG ones and put the corresponding HG parts there?

yes, provided you are consistent.

Sorry for the delay in the answer, other committments 😉

As Marco said, no you don't need a new record, that's not really a record, but rather the key used in the DropboxMetada dictionary, that's why you need to be consistent also when changing the DropboxMetada code.
So you can make a change as simple as:
TrackerAlignmentRcd -> TrackerAlignmentRcd_HG

About the DropboxMetadata you need to update:

ProduceDropBoxMetadata.py

DropBoxMetadataReader.py

add the needed json files in https://github.com/cms-sw/cmssw/tree/master/CondFormats/Common/data

in these json files you specify the name (and destination DB) of the tag to which PCL will append the new payloads

please note each workflow has 2 json files: one for prod and one for prep DB

Once that is done we (AlCa) will take care of creating the new DropboxMetada payload and append it to the data-taking tags before putting this PCL wf in production.

I hope I managed to find all the necessary changes, which are applied in b946b6b

tvami · 2022-06-21T14:14:49Z

type trk

tvami · 2022-06-21T14:15:32Z

urgent

the backport should go into the next 12_4_X

cmsbuild · 2022-06-21T18:00:42Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38449/30661

This PR adds an extra 48KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File Configuration/PyReleaseValidation/python/relval_steps.py modified in PR(s): Rawprime workflow with approximated siStrip clusters for heavy ions #38423, Update tau embed wf #38266

cmsbuild · 2022-06-21T18:01:08Z

Pull request #38449 was updated. @perrotta, @malbouis, @yuanchao, @jordan-martins, @bbilin, @tvami, @cmsbuild, @AdrianoDee, @srimanob, @kskovpen, @ggovi, @qliphy, @francescobrivio, @fabiocos, @davidlange6 can you please check and sign again.

tvami · 2022-06-23T17:14:09Z

@cms-sw/pdmv-l2 @cms-sw/upgrade-l2 could you please sign this? The backport should go into the 12_4_X that we'll cut next week, so it would be nice to have it aired in the IBs before that

bbilin · 2022-06-23T17:22:24Z

+1

srimanob · 2022-06-24T11:51:21Z

+Upgrade

From the upgrade side, existing workflow 1001.2 is updated. The test runs fine.

tvami · 2022-06-24T11:56:09Z

@cms-sw/orp-l2 we are essentially fully signed (and we'd like the backport to go into 12_4_1)

perrotta

Here are a few suggestion to improve the config and clean the code.
I don't want that by applying them the long list of signatures need to be recolectd: please update in a folllow up PR.

perrotta · 2022-06-24T13:17:02Z

Alignment/CommonAlignmentProducer/python/AlcaSiPixelAliHarvesterHG_cff.py

+        options = cms.vstring(
+            #'regularisation 1.0 0.05', # non-stated pre-sigma 50 mrad or 500 mum
+            'entries 500',
+            'chisqcut  30.0  4.5',
+            'threads 1 1',
+            'closeandreopen',
+            'skipemptycons' 
+            #'outlierdownweighting 3','dwfractioncut 0.1'
+            #'outlierdownweighting 5','dwfractioncut 0.2'
+        ),


Suggested change

options = cms.vstring(

#'regularisation 1.0 0.05', # non-stated pre-sigma 50 mrad or 500 mum

'entries 500',

'chisqcut 30.0 4.5',

'threads 1 1',

'closeandreopen',

'skipemptycons'

#'outlierdownweighting 3','dwfractioncut 0.1'

#'outlierdownweighting 5','dwfractioncut 0.2'

),

options = [

#'regularisation 1.0 0.05', # non-stated pre-sigma 50 mrad or 500 mum

'entries 500',

'chisqcut 30.0 4.5',

'threads 1 1',

'closeandreopen',

'skipemptycons'

#'outlierdownweighting 3','dwfractioncut 0.1'

#'outlierdownweighting 5','dwfractioncut 0.2' ],

Addressed at 1ac393c

perrotta · 2022-06-24T13:17:22Z

Alignment/CommonAlignmentProducer/python/AlcaSiPixelAliHarvesterHG_cff.py

+            #'outlierdownweighting 5','dwfractioncut 0.2'
+        ),
+        fileDir = 'HGalignment/',
+        runDir = cms.untracked.string('HGalignment/'),


Suggested change

runDir = cms.untracked.string('HGalignment/'),

runDir = 'HGalignment/',

Addressed at 1ac393c

perrotta · 2022-06-24T13:19:43Z

Alignment/CommonAlignmentProducer/src/AlignmentProducerBase.cc

@@ -854,7 +855,13 @@ void AlignmentProducerBase::writeForRunRange(cond::Time_t time) {
    auto alignments = alignableTracker_->alignments();
    auto alignmentErrors = alignableTracker_->alignmentErrors();
    this->writeDB(
-        alignments, "TrackerAlignmentRcd", alignmentErrors, "TrackerAlignmentErrorExtendedRcd", trackerGlobal, time);
+        // ~alignments, "TrackerAlignmentRcd", alignmentErrors, "TrackerAlignmentErrorExtendedRcd", trackerGlobal, time);


Can this commented out and outdated line get removed?

Addressed at 1ac393c

perrotta · 2022-06-24T13:33:05Z

CondFormats/PCLConfig/src/AlignPCLThresholdsHG.cc

+
+  if (it != floatMap_.end()) {
+    return true;
+  } else {
+    return false;
+  }


Suggested change

if (it != floatMap_.end()) {

return true;

} else {

return false;

}

return (it != floatMap_.end());

Addressed at 1ac393c

perrotta · 2022-06-24T13:36:25Z

+1

cmsbuild · 2022-06-24T13:36:45Z

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 be automatically merged.

perrotta · 2022-06-25T07:49:40Z

@dmeuser could you please have a look at the issue #38506 and provide a fix for it. It can be merged together with the cleanings I suggested in #38449 (review), and all them added together in the backport PR

Needless to say, this is rather urgent if we want to have this PR in the 12_4_1 release, that is expected to be built on Tuesday.

Attn. @mmusich @francescobrivio

dmeuser added 4 commits June 20, 2022 15:30

Method to set floatMap and check floatMap in new Threshold object

5355c8d

Use typedef map in new Threshold object

830554d

Implementation of new HG PCL alignment in alignment package

37df8ec

Add workflows for new HG PCL alignment

7d9d9c2

cmsbuild added this to the CMSSW_12_5_X milestone Jun 21, 2022

cmsbuild added alca-pending code-checks-pending db-pending operations-pending orp-pending pdmv-pending pending-signatures tests-pending upgrade-pending labels Jun 21, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Jun 21, 2022

mmusich reviewed Jun 21, 2022

View reviewed changes

francescobrivio reviewed Jun 21, 2022

View reviewed changes

cmsbuild added the trk label Jun 21, 2022

cmsbuild added urgent code-checks-pending and removed code-checks-approved labels Jun 21, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Jun 21, 2022

cmsbuild added pdmv-approved and removed pdmv-pending labels Jun 23, 2022

cmsbuild added upgrade-approved and removed upgrade-pending labels Jun 24, 2022

perrotta reviewed Jun 24, 2022

View reviewed changes

cmsbuild added fully-signed operations-approved orp-approved and removed operations-pending pending-signatures orp-pending labels Jun 24, 2022

cmsbuild merged commit 0f0c0f3 into cms-sw:master Jun 24, 2022

mmusich added a commit to cms-trackeralign/cmssw that referenced this pull request Jun 24, 2022

post-merge fixes for cms-sw#38449

1ac393c

mmusich mentioned this pull request Jun 24, 2022

[12.4.X] New high granularity pixel alignment for the PCL #38497

Merged

qliphy mentioned this pull request Jun 25, 2022

Unit failure: test_PixelBaryCentreTool had ERRORS #38506

Closed

mmusich mentioned this pull request Jun 25, 2022

Follow up to new high granularity pixel alignment for the PCL #38508

Merged

mmusich added a commit to cms-trackeralign/cmssw that referenced this pull request Jun 25, 2022

post-merge fixes for cms-sw#38449

6aec541

mmusich added a commit to cms-trackeralign/cmssw that referenced this pull request Jun 25, 2022

post-merge fixes for cms-sw#38449

e5d074e

mmusich added a commit to cms-trackeralign/cmssw that referenced this pull request Jun 25, 2022

post-merge fixes for cms-sw#38449

742cc9e

mmusich added a commit to cms-trackeralign/cmssw that referenced this pull request Jun 25, 2022

post-merge fixes for cms-sw#38449

c0c0d9f

mmusich added a commit to cms-trackeralign/cmssw that referenced this pull request Jun 25, 2022

post-merge fixes for cms-sw#38449

420403c

jhakala pushed a commit to jhakala/cmssw that referenced this pull request Aug 2, 2022

post-merge fixes for cms-sw#38449

3c2609b

jhakala pushed a commit to jhakala/cmssw that referenced this pull request Aug 3, 2022

post-merge fixes for cms-sw#38449

8de8f70

	inline const unsigned getPXBLadders(unsigned int lay) const { return m_pxbMap.at(lay).first; }
	inline const unsigned getPXBModules(unsigned int lay) const { return m_pxbMap.at(lay).second; }
	inline const unsigned getPXFBlades(int disk) const { return m_pxfMap.at(std::abs(disk)).first; }
	inline const unsigned getPXFModules(int disk) const { return m_pxfMap.at(std::abs(disk)).second; }

Name	start bit	hex mask	bit size	TrackerTopology method	Notes
subdetector part	23	0x3	2	pxfSide(id) or side(id)	1=FPIX- 2=FPIX+
Blade	12	0x3F	6	pxfBlade(id)	increasing phi and r: first inner ring blades and the outer ring blades
Panel	10	0x3	2	pxfPanel(id)	1=forward 2=backward
Module	2	0xFF	8	pxfModule(id)	always = 1

	runDir = cms.untracked.string('HGalignment/'),
	runDir = 'HGalignment/',

New high granularity pixel alignment for the PCL #38449

New high granularity pixel alignment for the PCL #38449

Conversation

dmeuser commented Jun 21, 2022

PR description:

PR validation:

cmsbuild commented Jun 21, 2022

cmsbuild commented Jun 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmusich Jun 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvami commented Jun 21, 2022

tvami commented Jun 21, 2022

cmsbuild commented Jun 21, 2022

cmsbuild commented Jun 21, 2022

tvami commented Jun 23, 2022

bbilin commented Jun 23, 2022

srimanob commented Jun 24, 2022

tvami commented Jun 24, 2022

perrotta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perrotta commented Jun 24, 2022

cmsbuild commented Jun 24, 2022

perrotta commented Jun 25, 2022

mmusich Jun 21, 2022 •

edited

Loading