[Draft] Add fillDescriptions to SiPixel digitizers #40673

tvami · 2023-02-02T15:02:54Z

PR description:

Opening a draft to discuss #40540 (comment)

PR validation:

in progress

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:

Not a backport and no backport is needed

resolves #40540

cmsbuild · 2023-02-02T15:10:00Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40673/34018

This PR adds an extra 48KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40673/34018/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40673/34018/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2023-02-03T19:40:11Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40673/34048

This PR adds an extra 24KB to repository

cmsbuild · 2023-02-03T19:40:37Z

A new Pull Request was created by @tvami (Tamas Vami) for master.

It involves the following packages:

SimGeneral/MixingModule (simulation)
SimTracker/Common (simulation)
SimTracker/SiPhase2Digitizer (upgrade, simulation)

@cmsbuild, @AdrianoDee, @srimanob, @civanch, @mdhildreth can you please review it and eventually sign? Thanks.
@echabert, @fabiocos, @VourMa, @makortel, @GiacomoSguazzoni, @robervalwalsh, @gbenelli, @rovere, @JanFSchulte, @missirol, @prolay, @threus, @mmusich, @VinInn, @mtosi, @dgulhan, @slomeo this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

tvami · 2023-02-03T19:53:51Z

@cmsbuild , please test

cmsbuild · 2023-02-03T20:01:39Z

-1

Failed Tests: Build HeaderConsistency ClangBuild
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4a8569/30391/summary.html
COMMIT: 52f65e4
CMSSW: CMSSW_13_0_X_2023-02-03-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40673/30391/install.sh to create a dev area with all the needed externals and cmssw changes.

Build

I found compilation error when building:

>> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_0_X_2023-02-03-1100/src/SimTracker/SiPixelDigitizer/plugins/SiPixelDigitizer.cc
>> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_0_X_2023-02-03-1100/src/SimTracker/SiPixelDigitizer/plugins/SealModule.cc
>> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_0_X_2023-02-03-1100/src/SimTracker/SiPixelDigitizer/plugins/PreMixingSiPixelWorker.cc
>> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_0_X_2023-02-03-1100/src/SimTracker/SiPixelDigitizer/plugins/SiPixelDigitizerAlgorithm.cc
In file included from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_0_X_2023-02-03-1100/src/SimTracker/SiPixelDigitizer/plugins/SiPixelDigitizerAlgorithm.cc:87:
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_0_X_2023-02-03-1100/src/SimTracker/Common/interface/SiPixelChargeReweightingAlgorithm.h:856:8: error: duplicate 'inline'
  856 | inline inline void SiPixelChargeReweightingAlgorithm::fillPSetDescription(edm::ParameterSetDescription& desc) {
      |        ^~~~~~
      |        ------
gmake: *** [tmp/el8_amd64_gcc11/src/SimTracker/SiPixelDigitizer/plugins/SimTrackerSiPixelDigitizerPlugins/SiPixelDigitizerAlgorithm.cc.o] Error 1
>> Building edm plugin tmp/el8_amd64_gcc11/src/SimTracker/SiPixelDigitizer/plugins/SimTrackerSiPixelDigitizerPlugins/libSimTrackerSiPixelDigitizerPlugins.so

Clang Build

I found compilation warning while trying to compile with clang. Command used:

USER_CUDA_FLAGS='--expt-relaxed-constexpr' USER_CXXFLAGS='-Wno-register -fsyntax-only' scram build -k -j 32 COMPILER='llvm compile'

See details on the summary page.

tvami · 2023-02-03T20:03:08Z

so this is funny
#40673 (comment)
because it was the code checks that made this
52f65e4

makortel

A few remarks that may be known already.

SimGeneral/MixingModule/plugins/MixingModule.h

SimTracker/SiPhase2Digitizer/plugins/Phase2TrackerDigitizer.h

SimTracker/SiPhase2Digitizer/plugins/Phase2TrackerDigitizerAlgorithm.h

cmsbuild · 2023-02-03T20:45:43Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40673/34049

This PR adds an extra 48KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40673/34049/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40673/34049/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2023-02-03T20:51:49Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40673/34050

This PR adds an extra 48KB to repository

cmsbuild · 2023-02-03T20:52:29Z

Pull request #40673 was updated. @cmsbuild, @AdrianoDee, @srimanob, @civanch, @mdhildreth can you please check and sign again.

tvami · 2023-02-03T20:54:00Z

@cmsbuild , please test

cmsbuild · 2023-02-03T20:57:24Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40673/34051

This PR adds an extra 48KB to repository

tvami · 2023-02-03T22:37:46Z

@cmsbuild, please abort

there are errors already, no point in running the rest

tvami · 2023-02-03T22:39:24Z

@makortel I see the following error

----- Begin Fatal Exception 03-Feb-2023 22:16:52 CET-----------------------
An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
   [1] Constructing module: class=MixingModule label='mix'
Exception Message:
MissingParameter: Parameter 'TemplateIDnumerator' not found.
----- End Fatal Exception -------------------------------------------------

https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4a8569/30393/runTheMatrix-results/10042.0_ZMM_13+2017/step2_ZMM_13+2017.log

TemplateIDnumerator is indeed one of the things I mean to put into the fillPSetDescription, but apparently it didnt propagate correctly to the MixingModule

makortel · 2023-02-03T23:49:23Z

TemplateIDnumerator is indeed one of the things I mean to put into the fillPSetDescription, but apparently it didnt propagate correctly to the MixingModule

Right. The current code in this PR

cmssw/SimGeneral/MixingModule/plugins/MixingModule.cc

Lines 707 to 711 in ee676fe

    
           void MixingModule::fillDescriptions(edm::ConfigurationDescriptions& descriptions) { 
        
             edm::ParameterSetDescription desc; 
        
             desc.setAllowAnything(); 
        
             descriptions.addDefault(desc); 
        
           }

tells the configuration validation system that MixingModule accepts all parameters, and nothing should be injected in the PSet before passing it to the MixingModule constructor (which is exactly the behavior when a module does not define fillDescriptions() function).

In order for the helper plugin fillPSetDescription() functions to be effective, all the

edm::DigiAccumulatorMixModPluginFactory
MixingModule::fillDescriptions()
fillPSetDescription() of all helper plugins declared with DEFINE_DIGI_ACCUMULATOR()

need to be changed as described in https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideConfigurationValidationAndHelp#A_Plugin_Module_Using_Another_He
(the only(?) shortcut being that the helper plugin fillPSetDescription() can be implemented with setAllowAnything())

(and I'd still expect the cms.SecSource to cause some kind of trouble for this exercise)

tvami · 2023-02-04T02:21:11Z

OK, I'll slowly go through these, there is quite a lot of DEFINE_DIGI_ACCUMULATOR plugins, and they are certainly beyond Tracker DPG code.

tvami · 2023-06-08T23:10:29Z

I'm sorry, I dont think I'll ever converge on this PR, I'm changing institutes and this will be (even more) out of scope of my responsibilities, so I'm closing this draft PR for now

Add fillDescriptions to SiPixel digitizer

0d760c1

cmsbuild added this to the CMSSW_13_0_X milestone Feb 2, 2023

cmsbuild added simulation-pending pending-signatures tests-pending orp-pending upgrade-pending code-checks-pending trk labels Feb 2, 2023

tvami marked this pull request as draft February 2, 2023 15:03

cmsbuild added code-checks-rejected and removed code-checks-pending labels Feb 2, 2023

Code checks

52f65e4

cmsbuild added code-checks-pending and removed code-checks-rejected labels Feb 3, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 3, 2023

cmsbuild added tests-started and removed tests-pending labels Feb 3, 2023

cmsbuild added tests-rejected and removed tests-started labels Feb 3, 2023

makortel reviewed Feb 3, 2023

View reviewed changes

SimGeneral/MixingModule/plugins/MixingModule.h Outdated Show resolved Hide resolved

SimTracker/SiPhase2Digitizer/plugins/Phase2TrackerDigitizer.h Outdated Show resolved Hide resolved

SimTracker/SiPhase2Digitizer/plugins/Phase2TrackerDigitizerAlgorithm.h Outdated Show resolved Hide resolved

After Matti comments

5c1cad0

cmsbuild removed the tests-rejected label Feb 3, 2023

Code checks again

ee676fe

cmsbuild added code-checks-rejected and removed code-checks-pending labels Feb 3, 2023

cmsbuild added code-checks-pending code-checks-approved and removed code-checks-rejected code-checks-pending labels Feb 3, 2023

cmsbuild added tests-started and removed tests-pending labels Feb 3, 2023

cmsbuild added tests-pending and removed tests-started labels Feb 3, 2023

smuzaffar modified the milestones: CMSSW_13_0_X, CMSSW_13_1_X Feb 11, 2023

cmsbuild mentioned this pull request Feb 17, 2023

Removed unused variables in SiPixelChargeReweightingAlgorithm #40805

Merged

cmsbuild mentioned this pull request Mar 9, 2023

Phase2 Tracker Digitizer updated to have the possibility to used planer pixel algorithm in 3D pixel #41010

Merged

cmsbuild mentioned this pull request May 2, 2023

[CLANG] Fix warnings about set but unused variables #41488

Merged

smuzaffar modified the milestones: CMSSW_13_1_X, CMSSW_13_2_X May 4, 2023

cmsbuild mentioned this pull request May 11, 2023

use qScale in pixel cluster charge reweighting #41623

Merged

tvami closed this Jun 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Draft] Add fillDescriptions to SiPixel digitizers #40673

[Draft] Add fillDescriptions to SiPixel digitizers #40673

tvami commented Feb 2, 2023 •

edited

cmsbuild commented Feb 2, 2023

cmsbuild commented Feb 3, 2023

cmsbuild commented Feb 3, 2023

tvami commented Feb 3, 2023

cmsbuild commented Feb 3, 2023

tvami commented Feb 3, 2023

makortel left a comment

cmsbuild commented Feb 3, 2023

cmsbuild commented Feb 3, 2023

cmsbuild commented Feb 3, 2023

tvami commented Feb 3, 2023

cmsbuild commented Feb 3, 2023

tvami commented Feb 3, 2023

tvami commented Feb 3, 2023

makortel commented Feb 3, 2023

tvami commented Feb 4, 2023

tvami commented Jun 8, 2023

[Draft] Add fillDescriptions to SiPixel digitizers #40673

[Draft] Add fillDescriptions to SiPixel digitizers #40673

Conversation

tvami commented Feb 2, 2023 • edited

PR description:

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 commented Feb 2, 2023

cmsbuild commented Feb 3, 2023

cmsbuild commented Feb 3, 2023

tvami commented Feb 3, 2023

cmsbuild commented Feb 3, 2023

Build

Clang Build

tvami commented Feb 3, 2023

makortel left a comment

Choose a reason for hiding this comment

cmsbuild commented Feb 3, 2023

cmsbuild commented Feb 3, 2023

cmsbuild commented Feb 3, 2023

tvami commented Feb 3, 2023

cmsbuild commented Feb 3, 2023

tvami commented Feb 3, 2023

tvami commented Feb 3, 2023

makortel commented Feb 3, 2023

tvami commented Feb 4, 2023

tvami commented Jun 8, 2023

tvami commented Feb 2, 2023 •

edited