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

Extending SeedingHitSet & SeedGeneratorFromProtoTracksEDProducer #36184

Merged
merged 3 commits into from Jan 25, 2022

Conversation

AdrianoDee
Copy link
Contributor

@AdrianoDee AdrianoDee commented Nov 19, 2021

PR description:

This PR replaces the fixed size (4) hit container in the SeedingHitSet with an std::vector. The SeedGeneratorFromProtoTracksEDProducer is adapted accordingly. The former constructors are kept for backward-compatibility.

PR validation:

Technical PR but some regression expected at HLT level when pixel tracks are used as seeds and more hits are used (see also #36215).

Run3 HLT tracking single iteration results documented here.

All tests run.

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36184/26762

  • This PR adds an extra 16KB to repository

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

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36184/26763

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • RecoTracker/TkSeedGenerator (reconstruction)
  • RecoTracker/TkSeedingLayers (reconstruction)

@jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @ebrondol, @gpetruc, @mmusich, @mtosi, @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

@AdrianoDee
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

-1

Failed Tests: RelVals RelVals-INPUT AddOn
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-06775f/20633/summary.html
COMMIT: 9516f84
CMSSW: CMSSW_12_2_X_2021-11-19-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36184/20633/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals

  • 140.53140.53_RunHI2011+RunHI2011+RECOHID11+HARVESTDHI/step2_RunHI2011+RunHI2011+RECOHID11+HARVESTDHI.log
  • 136.793136.793_RunDoubleEG2017C+RunDoubleEG2017C+HLTDR2_2017+RECODR2_2017reHLT_skimDoubleEG_Prompt+HARVEST2017/step3_RunDoubleEG2017C+RunDoubleEG2017C+HLTDR2_2017+RECODR2_2017reHLT_skimDoubleEG_Prompt+HARVEST2017.log
  • 136.874136.874_RunEGamma2018C+RunEGamma2018C+HLTDR2_2018+RECODR2_2018reHLT_skimEGamma_Offline_L1TEgDQM+HARVEST2018_L1TEgDQM/step3_RunEGamma2018C+RunEGamma2018C+HLTDR2_2018+RECODR2_2018reHLT_skimEGamma_Offline_L1TEgDQM+HARVEST2018_L1TEgDQM.log
Expand to see more relval errors ...

RelVals-INPUT

  • 140.53140.53_RunHI2011+RunHI2011+RECOHID11+HARVESTDHI/step2_RunHI2011+RunHI2011+RECOHID11+HARVESTDHI.log
  • 140.56140.56_RunHI2018+RunHI2018+RECOHID18+HARVESTDHI18/step2_RunHI2018+RunHI2018+RECOHID18+HARVESTDHI18.log
  • 1004.01004.0_RunHI2011+RunHI2011+TIER0EXPHI+ALCAEXPHI+ALCAHARVD1HI+ALCAHARVD2HI+ALCAHARVD3HI+ALCAHARVD5HI/step2_RunHI2011+RunHI2011+TIER0EXPHI+ALCAEXPHI+ALCAHARVD1HI+ALCAHARVD2HI+ALCAHARVD3HI+ALCAHARVD5HI.log
Expand to see more relval errors ...

AddOn Tests

  • hlt_mc_GRuncmsRun /cvmfs/cms-ib.cern.ch/week1/slc7_amd64_gcc900/cms/cmssw-patch/CMSSW_12_2_X_2021-11-19-1100/src/HLTrigger/Configuration/test/OnLine_HLT_GRun.py realData=False globalTag=@ inputFiles=@ : FAILED - time: date Fri Nov 19 18:10:32 2021-date Fri Nov 19 18:01:30 2021 s - exit: 35584
  • hlt_mc_GRuncmsDriver.py RelVal -s HLT:GRun,RAW2DIGI,L1Reco,RECO --mc --scenario=pp -n 10 --conditions auto:run3_mc_GRun --relval 9000,50 --datatier "RAW-HLT-RECO" --eventcontent FEVTDEBUGHLT --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --era Run3 --processName=HLTRECO --filein file:RelVal_Raw_GRun_MC.root --fileout file:RelVal_Raw_GRun_MC_HLT_RECO.root : FAILED - time: date Fri Nov 19 18:10:32 2021-date Fri Nov 19 18:01:30 2021 s - exit: 35584
  • hlt_mc_HIoncmsRun /cvmfs/cms-ib.cern.ch/week1/slc7_amd64_gcc900/cms/cmssw-patch/CMSSW_12_2_X_2021-11-19-1100/src/HLTrigger/Configuration/test/OnLine_HLT_HIon.py realData=False globalTag=@ inputFiles=@ : FAILED - time: date Fri Nov 19 18:09:46 2021-date Fri Nov 19 18:01:32 2021 s - exit: 35584
Expand to see more addon errors ...

@AdrianoDee AdrianoDee marked this pull request as draft November 20, 2021 07:03
@slava77
Copy link
Contributor

slava77 commented Nov 22, 2021

@AdrianoDee
this is marked as a draft.
Please clarify your expectation for a review here.
Thanks.

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36184/26821

  • This PR adds an extra 16KB to repository

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

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36184/27800

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

Pull request #36184 was updated. @jpata, @cmsbuild, @clacaputo, @slava77 can you please check and sign again.

@AdrianoDee
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-06775f/21737/summary.html
COMMIT: 7ce4dd6
CMSSW: CMSSW_12_3_X_2022-01-14-1100/slc7_amd64_gcc10
Additional Tests: PROFILING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36184/21737/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: 9 differences found in the comparisons
  • DQMHistoTests: Total files compared: 43
  • DQMHistoTests: Total histograms compared: 3461659
  • DQMHistoTests: Total failures: 12167
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3449470
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 42 files compared)
  • Checked 181 log files, 42 edm output root files, 43 DQM output files
  • TriggerResults: found differences in 6 / 42 workflows

@slava77
Copy link
Contributor

slava77 commented Jan 17, 2022

+reconstruction

for #36184 7ce4dd6

  • code changes are in line with the PR description and the follow up review
  • jenkins tests pass and comparisons with the baseline show no relevant differences in the reco objects comparisons and some differences in the HLT tracking. The HLT tracking is now using seeds with more than 4 hits (since HLT menu development for 12_3_X #36563) and this PR is enabling use of those hits in tracking
  • Looking at the reco profiling results:
    • in phase-1 11834.21 there is about 0.07% increase in the CPU cost based on apparently relevant increases in CAHitNtupletEDProducerT, HitTripletEDProducerT, and MultiHitFromChi2EDProducer; these modules also add 0.25% to the total memory allocations (in the performance piecharts)
      • compared to tests as of 8403361 (based on std::array of size 16), where the memory allocations of the total increased by about 1.7% (I did not record the CPU cost increase there; but I guess it was not negligible)
    • in phase-2 23434.21 the impact is fractionally smaller : CPU in the seeding modules is up by about 0.02% of the total reco time; the memory allocations are up by .12% of the total

@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

@makortel
Copy link
Contributor

This PR might have caused a segfault in HitPairEDProducer, see #36804.

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

10 participants