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

CA Extension to strips #47090

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

bdanzi
Copy link

@bdanzi bdanzi commented Jan 13, 2025

PR description:

This PR should introduce the usage of strips layers to the offline reco and HLT Cellular Automaton (CA) usage (work done in 15_0_0_pre1):

  • the strip addition and the changes in performance are expected only if using the customizer customizeHLTforAlpakaStrip for HLT and stripNTupletFit for offline reco, final geometry (Geometry/CommonTopologies/interface/SimplePixelStripTopology.py) pairs is still work in progress
  • little gain in performance for the CA applied to pixel only due to the removal of minYsizeB1 and minYsizeB2 cuts + generalization of Kernel_simpleTripletCleaner
    Test on TTbar PU EOR3 TRK DPG v7:
    http://uaf-3.t2.ucsd.edu/~bdanzi/plots_pixelsOnlyValidation_AllCases/plots_hlt_hltPixel/effandfakePtEtaPhi.png where results in blue and red are produced with a cleaned 14_2_0 CMSSW release while the black one comes along with this PR changes

PR validation:

Before submitting I was checking

  • basic test procedure suggested in the CMSSW PR instructions, some of them still failing with exception like:
A std::exception was thrown.
Connection on "frontier://(preferipfamily=0)(proxyconfigurl=http://grid-wpad/wpad.dat)(backupproxyurl=http://cmst0frontier.cern.ch:3128)(backupproxyurl\
=http://cmst0frontier1.cern.ch:3128)(backupproxyurl=http://cmst0frontier2.cern.ch:3128)(backupproxyurl=http://cmsbpfrontier.cern.ch:3128)(backupproxyur\
l=http://cmsbpfrontier1.cern.ch:3128)(backupproxyurl=http://cmsbpfrontier2.cern.ch:3128)(backupproxyurl=http://cmsbproxy.fnal.gov:3128)(serverurl=http:\
//cmsfrontier.cern.ch:8000/FrontierProd)(serverurl=http://cmsfrontier1.cern.ch:8000/FrontierProd)(serverurl=http://cmsfrontier2.cern.ch:8000/FrontierPr\
od)(serverurl=http://cmsfrontier3.cern.ch:8000/FrontierProd)(serverurl=http://cmsfrontier4.cern.ch:8000/FrontierProd)/CMS_CONDITIONS" cannot be establi\
shed ( CORAL : "ConnectionPool::getSessionFromNewConnection" from "CORAL/Services/ConnectionService" )
----- End Fatal Exception -------------------------------------------------

cc @mmasciov @slava77
Work co-authored-by: @AdrianoDee Adriano Di Florio [email protected]

@cmsbuild cmsbuild changed the base branch from CMSSW_15_0_X to master January 13, 2025 09:47
@cmsbuild
Copy link
Contributor

bdanzi, CMSSW_15_0_X branch is closed for direct updates. cms-bot is going to move this PR to master branch.
In future, please use cmssw master branch to submit your changes.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 13, 2025

cms-bot internal usage

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47090/43269

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-47090/43271

@cmsbuild
Copy link
Contributor

Pull request #47090 was updated.

@AdrianoDee
Copy link
Contributor

please test

@AdrianoDee
Copy link
Contributor

@cmsbuild alllow @bdanzi test rights

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47090/43280

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

@AdrianoDee
Copy link
Contributor

@cmsbuild allow @bdanzi test rights

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47090/43281

@cmsbuild
Copy link
Contributor

Pull request #47090 was updated.

@bdanzi
Copy link
Author

bdanzi commented Jan 14, 2025

please test

@mmusich
Copy link
Contributor

mmusich commented Jan 14, 2025

@smuzaffar, looks like #47090 (comment) didn't have the desired effect. Is it a syntax issue, or does the PR need to be "open for review" to allow it?

@smuzaffar
Copy link
Contributor

@mmusich , it should be allow user test rights ( i.e. without starting @cmsbuild). For now, please fix the comment. I will update bot code to recognize optional @cmsbuild, too

@mmusich
Copy link
Contributor

mmusich commented Jan 14, 2025

allow @bdanzi test rights

@mmusich
Copy link
Contributor

mmusich commented Jan 14, 2025

For now, please fix the comment. I will update bot code to recognize optional @cmsbuild, too

thanks @smuzaffar !

@bdanzi
Copy link
Author

bdanzi commented Jan 14, 2025

please test

@cmsbuild
Copy link
Contributor

-1

Failed Tests: UnitTests
Size: This PR adds an extra 116KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ff55b0/43754/summary.html
COMMIT: 508a7fd
CMSSW: CMSSW_15_0_X_2025-01-13-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/47090/43754/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found 1 errors in the following unit tests:

---> test test-das-selected-lumis had ERRORS

Comparison Summary

Summary:

  • You potentially added 33 lines to the logs
  • Reco comparison results: 38 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3932183
  • DQMHistoTests: Total failures: 37127
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3895036
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 218 log files, 189 edm output root files, 50 DQM output files
  • TriggerResults: found differences in 9 / 48 workflows

@mmusich
Copy link
Contributor

mmusich commented Jan 14, 2025

enable gpu


# CMSSW version specific customizations
def customizeHLTforCMSSW(process, menuType="GRun"):

process = customiseForOffline(process)

process = configureFrameSoAESProducers(process)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this customisation which is always executed? Is it purely technical (no changes in physics expected), or making CA extension changes?
And what is the relation to the above customisation file customizeHLTforAlpakaStripNoDoubletRecovery.py which isn't called explicitly anywhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this customisation which is always executed? Is it purely technical (no changes in physics expected),

without it, the HLT processing fails:

----- Begin Fatal Exception 15-Jan-2025 10:53:31 CET-----------------------
An exception of category 'NoRecord' occurred while
   [0] Processing  Event run: 386593 lumi: 235 event: 535875922 stream: 0
   [1] Running path 'AlCa_PFJet40_CPUOnly_v11'
   [2] Calling method for module alpaka_serial_sync::CAHitNtupletAlpakaPhase1/'hltPixelTracksSoASerialSync'
Exception Message:
No "FrameSoARecord" record found in the EventSetup.

 Please add an ESSource or ESProducer that delivers such a record.
----- End Fatal Exception -------------------------------------------------

I suppose it's for compatibility reasons (if the customization is not present then the seeding module expects an ES product which is not available).
On the other hand looking at the trigger results there are trigger decision changes, so the PR as it is it's not transparent.

And what is the relation to the above customisation file customizeHLTforAlpakaStripNoDoubletRecovery.py which isn't called explicitly anywhere?

I am assuming customizeHLTforAlpakaStripNoDoubletRecovery is the "meaty" part of this PR (the one actually making the physics changes) -- though I agree it's not called anywhere and that's not good because we need a way to test it.

@bdanzi @cms-sw/tracking-pog-l2 please elaborate / clarify.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, enabling customizeHLTforAlpakaStripNoDoubletRecovery would be the ultimate goal following this PR.
As is, this customization is introduced to enable the possibility for other downstream users to test it, with the idea to possibly enable it afterwards. Are you suggesting that this is called in a special workflow?

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you suggesting that this is called in a special workflow?

IMHO that would be useful for reviewing purposes.

Copy link
Author

@bdanzi bdanzi Jan 15, 2025

Choose a reason for hiding this comment

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

For physics changes outside the customizeHLTforAlpakaStripNoDoubletRecovery, I guess those are introduced due to the lowering of cuts on minYsizeB1 and minYsizeB2 in Geometry/CommonTopologies/interface/SimplePixelTopology.h that affects also the CA applied only on pixels since I was observing better performance with minor increase in fake+duplicates, especially in EOR3 scenario + generalization of Kernel_simpleTripletCleaner

@jfernan2
Copy link
Contributor

enable profiling

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