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

Move module loop to setup #175

Merged
merged 4 commits into from
Sep 22, 2022
Merged

Move module loop to setup #175

merged 4 commits into from
Sep 22, 2022

Conversation

Jingyan95
Copy link

Moving expensive (module) loops to Setup.cc which is called at BeginRun.

@tomalin
Copy link
Collaborator

tomalin commented Jul 19, 2022

Please say in Description if you've checked this reduces CPU use.

@tomalin
Copy link
Collaborator

tomalin commented Jul 19, 2022

I believe the HPH is used only in L1TrackNtupleMaker.cc to calculate variables such as tmp_trk_nPSstub_hitpattern that are added to the TTree? So if HPH contains bugs, they won't show up (except if compilation errors) in the CI test checks of tracking efficiency? Have you therefore checked that these variables look sensible for both NewKF & OldKF?

@tomalin
Copy link
Collaborator

tomalin commented Jul 22, 2022

The Hybrid code crashed in the CI test https://gitlab.cern.ch/cms-l1tk/cmssw_CI/-/pipelines/4155053 .

@Jingyan95
Copy link
Author

@tomalin I moved the HPH files under TrackFindingTracklet package such that Thomas's functions from TrackerTFP can be included without creating circular dependency.

@Jingyan95
Copy link
Author

Hi @tomalin , I wanted to check if you had some time to look at this PR ? The primary comment (CPU usage) was addressed by the recent commit and the hitpatternhelper module now uses very minimal CPU.

@tomalin
Copy link
Collaborator

tomalin commented Sep 2, 2022

@tomalin I moved the HPH files under TrackFindingTracklet package such that Thomas's functions from TrackerTFP can be included without creating circular dependency.

@Jingyan95 Can you please explicitely answer each of the review comments. Whilst some remain unanswered, I assume the work's not yet been done, so don't bother rereviewing.

@Jingyan95
Copy link
Author

Please say in Description if you've checked this reduces CPU use.

CPU usage is negligible now

@Jingyan95
Copy link
Author

Jingyan95 commented Sep 9, 2022

I believe the HPH is used only in L1TrackNtupleMaker.cc to calculate variables such as tmp_trk_nPSstub_hitpattern that are added to the TTree? So if HPH contains bugs, they won't show up (except if compilation errors) in the CI test checks of tracking efficiency? Have you therefore checked that these variables look sensible for both NewKF & OldKF?

The performance of the HPH has been checked and it is consistent with the previous version
Old KF:
TTbar_PU200_D76_FPR_hitpattern.pdf
TTbar_PU200_D76_TPR_hitpattern.pdf
New KF:
TTbar_PU200_D76_NEWKF_FPR_hitpattern.pdf
TTbar_PU200_D76_NEWKF_TPR_hitpattern.pdf

@skinnari
Copy link

hi @tomalin @Jingyan95 it seems that all comments have been addressed, can this PR be merged? thanks!

Copy link
Collaborator

@tomalin tomalin left a comment

Choose a reason for hiding this comment

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

approved

@tomalin tomalin merged commit 205b191 into L1TK-dev-12_5_0_pre2 Sep 22, 2022
dally96 pushed a commit that referenced this pull request Nov 1, 2022
* Move module loop to setup

* Import functions from trackerTFP

* Ian's 2nd comment

* Code-format

Co-authored-by: Jack Li <[email protected]>
dally96 pushed a commit that referenced this pull request Nov 2, 2022
* Move module loop to setup

* Import functions from trackerTFP

* Ian's 2nd comment

* Code-format

Co-authored-by: Jack Li <[email protected]>
tomalin pushed a commit that referenced this pull request Nov 24, 2022
* Move module loop to setup

* Import functions from trackerTFP

* Ian's 2nd comment

* Code-format

Co-authored-by: Jack Li <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants