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

Phase2 HLT pixel modules duplicate removal #42491

Merged
merged 13 commits into from
Aug 8, 2023

Conversation

lguzzi
Copy link
Contributor

@lguzzi lguzzi commented Aug 7, 2023

PR description:

This PR removes some duplicated modules in the phase2 HLT menu and renames some of the pixel modules. See here for more details.

Details of the PR:

  • hltPhase2L3MuonPixelFitterByHelixProjections and hltPhase2L3MuonPixelTracksFitter are removed in favour of pixelFitterByHelixProjections. These modules are fully duplicated and no change is expected.
  • pixelFitterByHelixProjections is renamed to hltPhase2PixelFitterByHelixProjections
  • hltPhase2L3MuonPixelTrackFilterByKinematics and hltPhase2L3MuonPixelTracksFilter are removed in favour of pixelTrackFilterByKinematics. These modules are fully duplicated and no change is expected.
  • pixelTrackFilterByKinematics is renamed to hltPhase2PixelTrackFilterByKinematics
  • caloTowerForTrk is removed in favour of hltTowerMakerForAll. No change is expected, more info here
  • hltTowerMakerForAll is renamed to hltPhase2TowerMakerForAll
  • highPtTripletStepTrackingRegions is removed in favour of pixelTracksTrackingRegions. No change is expected, more info here
  • pixelTracksTrackingRegions is renamed to hltPhase2PixelTracksAndHighPtStepTrackingRegions
  • pixelTracksHitDoublets is renamed to hltPhase2PixelTracksHitDoublets
  • pixelTracksHitSeeds is renamed to hltPhase2PixelTracksHitSeeds
  • pixelTracksSeedLayers is renamed to hltPhase2PixelTracksSeedLayers
  • pixelTracks is renamed to hltPhase2PixelTracks
  • pixelVertices is renamed to hltPhase2PixelVertices
  • pixelTracksTask is renamed to hltPhase2PixelTracksTask

No change is expected.

PR validation:

Tested the commits privately running

cmsDriver.py Phase2 -s HLT:75e33 --processName=HLTX \
  --conditions auto:phase2_realistic_T21 \
  --geometry Extended2026D95 \
  --era Phase2C17I13M9 \
  --customise SLHCUpgradeSimulations/Configuration/aging.customise_aging_1000 \
  --eventcontent FEVTDEBUGHLT \
  --filein /store/relval/CMSSW_13_2_0_pre3/RelValTTbar_14TeV/GEN-SIM-DIGI-RAW/131X_mcRun4_realistic_v6_2026D98noPU-v1/2580000/01b21771-37d5-41c2-b0ea-97bfce125a9a.root \
  -n 10 --nThreads 4

on 13_3_X as from the phase2 HLT twiki. Output: result on 133X (baseline result run on c2e865a for reference)

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. To be backported to 13_1_X and 13_2_X for the phase2 hlt menu.

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 7, 2023

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42491/36496

  • This PR adds an extra 64KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 7, 2023

A new Pull Request was created by @lguzzi (Luca Guzzi) for master.

It involves the following packages:

  • HLTrigger/Configuration (hlt)

@cmsbuild, @missirol, @Martin-Grunewald can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @missirol, @silviodonato, @SohamBhattacharya, @beaucero 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

@slava77
Copy link
Contributor

slava77 commented Aug 7, 2023

type tracking

@rovere
Copy link
Contributor

rovere commented Aug 7, 2023

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 7, 2023

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a44b1f/34135/summary.html
COMMIT: fe8af11
CMSSW: CMSSW_13_3_X_2023-08-07-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/42491/34135/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 3 lines from the logs
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 133 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3150947
  • DQMHistoTests: Total failures: 6
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3150919
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 207 log files, 159 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@mmusich
Copy link
Contributor

mmusich commented Aug 7, 2023

@lguzzi

Reco comparison results: 133 differences found in the comparisons

Are these expected?

@lguzzi
Copy link
Contributor Author

lguzzi commented Aug 8, 2023

@mmusich ,
I am not familiar with these tests, but if I read this right,
within the trk phase2 upgrade group we decided to rename the phase 2 modules and collections from pixel[...] to hltPhase2Pixel[...] (see here), this includes also pixelTracks_cfi and pixelVertices_cfi.
From the summary I see that the failing tests refer all to pixel tracks and vertices quantities. The test seems to fail to collect any information for those collections. I believe this is expected because these collections are now hltPhase2Pixel[Tracks/Vertices].
Within the HLT_75e33 menu, all paths/tasks/modules are up to date with the new naming scheme.

@mmusich
Copy link
Contributor

mmusich commented Aug 8, 2023

From the summary I see that the failing tests refer all to pixel tracks and vertices quantities. The test seems to fail to collect any information for those collections.

I am wondering if it is accidental that the Reco comparison is capturing these events products (since the name they have matched somewhat the offline nomenclature). In any case in lack of an appropriate Phase-2 DQM @ HLT (#39362), I guess this doubled back as additional check. Not sure if the script should be updated using the new names.

@missirol
Copy link
Contributor

missirol commented Aug 8, 2023

@beaucero @SohamBhattacharya @rovere , do you have any comments on this PR ? If not, I can sign it.

@SohamBhattacharya
Copy link
Contributor

@beaucero @SohamBhattacharya @rovere , do you have any comments on this PR ? If not, I can sign it.

No comments from me. Thanks.

@missirol
Copy link
Contributor

missirol commented Aug 8, 2023

+hlt

  • "Reco comparison" differences are due to the changes to the Phase-2 HLT menu (seen from the process name "HLT" in the name of the plots).
  • Probably, those HLT collections were previously picked up in those DQM plots because named as their offline counterparts.
  • This accidental monitoring is now lost, and clearly Add Validation and DQM to Phase2 HLT WF #39362 is not ready to be closed.

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 8, 2023

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

@perrotta
Copy link
Contributor

perrotta commented Aug 8, 2023

+1

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.

8 participants