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 process modifier to enable x-talk in Phase-2 InnerTracker pixels #37028

Merged

Conversation

emiglior
Copy link
Contributor

PR description:

This PR concerns the phase-2 upgrade.
This PR introduces a process modifier to activate cross-talk in the phase-2 Inner Tracker 25x100 um planar pixels.
For the time being, the process modifier is meant to be used only as a command line option of cmsDriver.py for requesting RelVals to study the impact of cross-talk.
Workflows in upgradeWorkflowComponents.py are not affected by this process modifier.
As no mitigation of cross-talk is currently implemented in the local reco of phase-2 Inner Tracker, we plan to request RelVals up to the DIGI step.

PR validation:

  • Checked runTheMatrix workflows run without errors
  • Checked running w/f 36200.0 without and with the "--procModifier enableXTalkInPhase2Pixel" option that the distribution of DIGI in the InnerTracker changes as expected

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

This PR is not a backport

@suchandradutta @AndreasAlbert

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37028/28456

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @emiglior (Ernesto Migliore) for master.

It involves the following packages:

  • Configuration/ProcessModifiers (operations)
  • SimTracker/SiPhase2Digitizer (upgrade, simulation)

@perrotta, @civanch, @mdhildreth, @cmsbuild, @AdrianoDee, @srimanob, @qliphy, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks.
@fabiocos, @makortel, @mtosi, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @Martin-Grunewald, @missirol, @mmusich, @threus, @dgulhan 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

@srimanob
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-98c3fa/22577/summary.html
COMMIT: 8b11eb1
CMSSW: CMSSW_12_3_X_2022-02-22-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37028/22577/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3965143
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3965119
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 204 log files, 45 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@emiglior
Copy link
Contributor Author

@OzAmram FYI

@civanch
Copy link
Contributor

civanch commented Feb 23, 2022

@emiglior , is it possible to edit the title: ph2 -> Phase-2 ?

@emiglior emiglior changed the title add process modifier to enable x-talk in ph2 InnerTracker pixels add process modifier to enable x-talk in Phase-2 InnerTracker pixels Feb 23, 2022
@civanch
Copy link
Contributor

civanch commented Feb 23, 2022

+1

@emiglior
Copy link
Contributor Author

Please assign this PR to upgrade and test it.
As the process modifier is supposed to have no effect on the existing w/f, any w/f with premixing should be ok for the test,
e.g. 39634.99

@srimanob
Copy link
Contributor

Hi @emiglior
The PR test is done with D77 premixing
35234.999_TTbar_14TeV+2026D77PU_PMXS1S2PR+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+PREMIX_PremixHLBeamSpot14PU+DigiTriggerPU+RecoGlobalPU+HARVESTGlobalPU/

Is this not enough?

@srimanob
Copy link
Contributor

test parameters:

  • workflows = 39634.999
  • relvals_opt = --what upgrade,standard,highstats,pileup,generator,extendedgen,production,ged,machine,premix

@srimanob
Copy link
Contributor

@cmsbuild please test

@srimanob
Copy link
Contributor

By the way, testing with 39634.99, we will not have a comparison.

@emiglior
Copy link
Contributor Author

@srimanob as it should have no effects on the existing w/f, please feel free to use any other process with preMixing as benchmark

@mmusich
Copy link
Contributor

mmusich commented Feb 24, 2022

Please assign this PR to upgrade and test it.
As the process modifier is supposed to have no effect on the existing w/f, any w/f with premixing should be ok for the test,
e.g. 39634.99

I am a bit confused by these requests:

  • the PR is already assigned to upgrade
  • as there is no wf in the matrix that makes use of the modifier introduced here, what is the purpose of asking a specific test? The regular matrix was already tested successfully here

@cmsbuild
Copy link
Contributor

-1

Failed Tests: RelVals
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-98c3fa/22633/summary.html
COMMIT: 8b11eb1
CMSSW: CMSSW_12_3_X_2022-02-23-2300/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37028/22633/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals

----- Begin Fatal Exception 24-Feb-2022 09:36:14 CET-----------------------
An exception of category 'NoSecondaryFiles' occurred while
   [0] Constructing the EventProcessor
   [1] Constructing module: class=MixingModule label='mix'
Exception Message:
RootEmbeddedFileSequence no input files specified for secondary input source.
----- End Fatal Exception -------------------------------------------------

@srimanob
Copy link
Contributor

test parameters:

@srimanob
Copy link
Contributor

srimanob commented Feb 24, 2022

I agree with @mmusich as I asked if 35234.999 is not good enough. I will retrigger the standard test to get green light, and then I will sign the PR.

@srimanob
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-98c3fa/22635/summary.html
COMMIT: 8b11eb1
CMSSW: CMSSW_12_3_X_2022-02-23-2300/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37028/22635/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 4001143
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 4001119
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 204 log files, 45 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@srimanob
Copy link
Contributor

+Upgrade

This PR introduces proc modifier which is not turned on by default. No change is expected, and not seen in the PR test.

@perrotta
Copy link
Contributor

+operations

@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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@perrotta
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 7107849 into cms-sw:master Feb 27, 2022
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.

6 participants