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

Run 2, Run3, and Phase2 tracker: adding T27 (geometry with bricked pixels) and support for storing bricked pixel information #34120

Merged
merged 6 commits into from Jul 28, 2021

Conversation

adewit
Copy link
Contributor

@adewit adewit commented Jun 14, 2021

PR description:

This adds a phase 2 tracker geometry (T27) containing bricked pixels in TEPX&TFPX, as well as the central 1 (3) rod(s) of TBPX L2 (L3 and 4). Because the information about whether a pixel is "bricked" or not should be available at the level of the geometry, a new record to store this information has also been added, as discussed in these slides. Note that wrt that presentation, the record has been renamed PTrackerAdditionalParametersPerDetRcd, and it has been made expandable to make it possible to add further bool/int/float per-detID information for the tracker in the future.
For now the record is created from the XML, as no Phase 2 geometries are stored in the DB, but a DB writer is also included for future use.

NOTE: the new record PTrackerAdditionalParametersPerDetRcd is consumed by the TrackerGeomBuilderFromGeometricDet, which is the same for Runs 2&3 and Phase 2. Therefore this PR also modifies Runs 2&3, by including the new record, which however only contains the default value (False) for the brickedness boolean, as this parameter of course does not exist before Phase 2

The new tracker geometry T27 that uses the bricked pixels is part of detector D85. The workflows for D85 only run up to and including the digitization step to avoid having to include changes in the reconstruction code at this point. NOTE: the modifications to the digitzer that are needed to handle bricked pixels are NOT included in this PR ( @franzglessgen will provide that in due course). In the current state the standard digitizer for planar pixels will be used; this is of course not the final configuration but it does not lead to problems.

@emiglior @mmusich fyi

PR validation:

  • Checked that the addition of the new rcd does not break non-Phase2 workflows (checked for a few workflows)
  • Checked that the information on the brickedness of the pixels is passed on as expected
  • Verified that the DB writer is working

if this PR is a backport please specify the original PR and why you need to backport that PR:

Not a backport

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34120/23314

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

Alignment/CommonAlignmentMonitor
Alignment/CommonAlignmentProducer
Alignment/LaserAlignment
Alignment/MillePedeAlignmentAlgorithm
Alignment/OfflineValidation
Alignment/SurveyAnalysis
Alignment/TrackerAlignment
CondCore/GeometryPlugins
CondCore/Utilities
CondFormats/GeometryObjects
CondTools/Geometry
Configuration/AlCa
Configuration/Geometry
Configuration/PyReleaseValidation
Configuration/StandardSequences
Geometry/CMSCommonData
Geometry/CommonTopologies
Geometry/MTDGeometryBuilder
Geometry/Records
Geometry/TrackerCommonData
Geometry/TrackerGeometryBuilder
Geometry/TrackerNumberingBuilder

@chayanit, @wajidalikhan, @ianna, @kpedro88, @tlampen, @pohsun, @civanch, @yuanchao, @silviodonato, @cmsbuild, @davidlange6, @Dr15Jones, @cvuosalo, @mdhildreth, @ggovi, @qliphy, @fabiocos, @francescobrivio, @malbouis, @jordan-martins, @makortel, @srimanob can you please review it and eventually sign? Thanks.
@felicepantaleo, @ghugo83, @Martin-Grunewald, @tlampen, @mmusich, @slomeo, @venturia, @pakhotin, @vargasa, @makortel, @JanFSchulte, @dgulhan, @seemasharmafnal, @cvuosalo, @GiacomoSguazzoni, @rovere, @VinInn, @tocheng, @ebrondol, @mtosi, @fabiocos, @lecriste this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@mmusich
Copy link
Contributor

mmusich commented Jun 14, 2021

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

-1

Failed Tests: UnitTests AddOn
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7f0f25/15948/summary.html
COMMIT: 7213ad3
CMSSW: CMSSW_12_0_X_2021-06-14-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/34120/15948/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS Clang-Tidy warnings: There are Clang-Tidy warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7f0f25/15948/llvm-analysis/cmsclangtidy.txt for details.

Unit Tests

I found errors in the following unit tests:

---> test GeometryDOMCount had ERRORS
---> test GeometryTrackerGeometryBuilderTestDriver had ERRORS
---> test testTauEmbeddingProducers had ERRORS

AddOn Tests

----- Begin Fatal Exception 14-Jun-2021 23:49:32 CEST-----------------------
An exception of category 'NoRecord' occurred while
   [0] Running EventSetup component TrackerDigiGeometryESModule/'
   [1] While getting dependent Record from Record TrackerDigiGeometryRecord
Exception Message:
No "PTrackerAdditionalParametersPerDetRcd" record found in the EventSetup.n
 Please add an ESSource or ESProducer that delivers such a record.
----- End Fatal Exception -------------------------------------------------
----- Begin Fatal Exception 14-Jun-2021 23:50:07 CEST-----------------------
An exception of category 'NoRecord' occurred while
   [0] Running EventSetup component TrackerDigiGeometryESModule/'
   [1] While getting dependent Record from Record TrackerDigiGeometryRecord
Exception Message:
No "PTrackerAdditionalParametersPerDetRcd" record found in the EventSetup.n
 Please add an ESSource or ESProducer that delivers such a record.
----- End Fatal Exception -------------------------------------------------
----- Begin Fatal Exception 14-Jun-2021 23:50:10 CEST-----------------------
An exception of category 'NoRecord' occurred while
   [0] Running EventSetup component TrackerDigiGeometryESModule/'
   [1] While getting dependent Record from Record TrackerDigiGeometryRecord
Exception Message:
No "PTrackerAdditionalParametersPerDetRcd" record found in the EventSetup.n
 Please add an ESSource or ESProducer that delivers such a record.
----- End Fatal Exception -------------------------------------------------
Expand to see more addon errors ...

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 38
  • DQMHistoTests: Total histograms compared: 2862520
  • DQMHistoTests: Total failures: 7
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2862491
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 37 files compared)
  • Checked 160 log files, 37 edm output root files, 38 DQM output files
  • TriggerResults: no differences found

@mmusich
Copy link
Contributor

mmusich commented Jun 15, 2021

---> test testTauEmbeddingProducers had ERRORS

this is spurious and due to issue #34112.
All the rest of failures are real.

@adewit
Copy link
Contributor Author

adewit commented Jun 15, 2021

---> test testTauEmbeddingProducers had ERRORS

this is spurious and due to issue #34112.
All the rest of failures are real.

Thanks - I understand all the rest of them apart from the GeometryDOMCount one, which works locally :/

}

edm::ESTransientHandle<GeometricDet> gd;
es.get<IdealGeometryRecord>().get(gd);
Copy link
Contributor

Choose a reason for hiding this comment

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

please use esConsumes here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -4001,6 +4001,7 @@
applyAlignment = cms.bool( True ),
alignmentsLabel = cms.string( "" )
)
process.TrackerAdditionalParametersPerDetESModule = cms.ESProducer("TrackerAdditionalParametersPerDetESModule")
Copy link
Contributor

Choose a reason for hiding this comment

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

@adewit, direct modification of the HLT menu are disallowed as per TSG policy.
Please add a customization function in customizeHLTforCMSSW

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@adewit
Copy link
Contributor Author

adewit commented Jun 15, 2021

---> test GeometryDOMCount had ERRORS

After investigation I don't think this one is related to this PR; the IB CMSSW_12_0_X_2021-06-14-1100 is missing the package DetectorDescription/Schema, and that's why this test fails.

@mmusich
Copy link
Contributor

mmusich commented Jul 28, 2021

and (2) that the PR #34542 got merged which now leads to a merge conflict.

@tvami I don't see any merge conflict:

image

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7f0f25/17287/summary.html
COMMIT: c05d615
CMSSW: CMSSW_12_0_X_2021-07-27-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/34120/17287/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS StaticAnalyzer warnings: There are 1 inherits from legacy modules warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7f0f25/17287/llvm-analysis/legacy-mod-sa.txt for details.

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-7f0f25/38634.0_TTbar_14TeV+2026D86+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-7f0f25/39034.0_TTbar_14TeV+2026D87+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 6 differences found in the comparisons
  • DQMHistoTests: Total files compared: 39
  • DQMHistoTests: Total histograms compared: 2998564
  • DQMHistoTests: Total failures: 12
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 2998529
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 38 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 165 log files, 37 edm output root files, 39 DQM output files
  • TriggerResults: no differences found

@srimanob
Copy link
Contributor

For the recording, do you plan to fix the warning,
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7f0f25/17287/llvm-analysis/legacy-mod-sa.txt
in later PR? Sorry, it was mentioned somewhere in this long PR.

@adewit @mmusich

@srimanob
Copy link
Contributor

srimanob commented Jul 28, 2021

@cms-sw/db-l2 Could you please consider and sign if everything is OK from your side? Big thanks.

@ggovi
Copy link
Contributor

ggovi commented Jul 28, 2021

+1

@qliphy
Copy link
Contributor

qliphy commented Jul 28, 2021

+operations

@qliphy
Copy link
Contributor

qliphy commented Jul 28, 2021

+1

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

@mmusich
Copy link
Contributor

mmusich commented Jul 29, 2021

@srimanob

For the recording, do you plan to fix the warning,
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7f0f25/17287/llvm-analysis/legacy-mod-sa.txt
in later PR? Sorry, it was mentioned somewhere in this long PR.

Addressed at #34690

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