Skip to content
This repository has been archived by the owner on Dec 9, 2024. It is now read-only.

T3 remove redundant new #393

Merged
merged 29 commits into from
May 7, 2024
Merged

T3 remove redundant new #393

merged 29 commits into from
May 7, 2024

Conversation

YonsiG
Copy link
Contributor

@YonsiG YonsiG commented Apr 29, 2024

In the T3 kernels, we have some functions called runTripletDefaultAlgoBBBB, and also the BBEE and EEEE regions. Those cuts are duplicating with the passPointingConstraintBBB ... functions. On the other hand, introducing beta iteration cuts to triplet objects are not physically meaningful. In this PR, we still keep the first beta cut as geometric requirement and move to passPointingConstraint function, but remove all the beta iteration functions.

Another improvement is that the triplet radius is calculated and saved for each of the T3s. In this case, we don't need to calculate the radius of triplet multiple times when building T5, pT3 and pT5s. This will also bring timing improvement.
Slides describing the influence of this PR is summarized here.
https://indico.cern.ch/event/1407246/contributions/5914814/attachments/2846057/4976175/check%20the%20T3%20cuts%20EEEE%202.pdf

@YonsiG
Copy link
Contributor Author

YonsiG commented Apr 29, 2024

** closed the previous PR with thread divergence. **
created a new one. Addressing Slava's first comments already.

@YonsiG YonsiG marked this pull request as draft April 29, 2024 14:51
@YonsiG
Copy link
Contributor Author

YonsiG commented Apr 29, 2024

/run standalone
/run CMSSW

@YonsiG YonsiG marked this pull request as ready for review April 29, 2024 20:36
@SegmentLinking SegmentLinking deleted a comment from github-actions bot Apr 29, 2024
@SegmentLinking SegmentLinking deleted a comment from github-actions bot Apr 29, 2024
Copy link

The PR was built and ran successfully with CMSSW. Here are some plots.

OOTB All Tracks
Efficiency and fake rate vs pT, eta, and phi

The full set of validation and comparison plots can be found here.

@YonsiG
Copy link
Contributor Author

YonsiG commented Apr 30, 2024

Merging this PR will be able to close Issue #214 and Issue #247 related to triplet building cuts

@YonsiG YonsiG force-pushed the T3_removeRedundant_new branch from ff0c90a to 87d4bfc Compare May 1, 2024 15:05
@YonsiG
Copy link
Contributor Author

YonsiG commented May 1, 2024

/run standalone

Copy link

github-actions bot commented May 1, 2024

The PR was built and ran successfully in standalone mode. Here are some of the comparison plots.

Efficiency vs pT comparison Efficiency vs eta comparison
Fake rate vs pT comparison Fake rate vs eta comparison
Duplicate rate vs pT comparison Duplicate rate vs eta comparison

The full set of validation and comparison plots can be found here.

Here is a timing comparison:

   Evt    Hits       MD       LS      T3       T5       pLS       pT5      pT3      TC       Reset    Event     Short             Rate
   avg     43.8    323.0    122.9     69.7     91.5    492.8    128.5    157.5     98.3      3.3    1531.3     994.7+/- 267.6     418.3   explicit_cache[s=4] (master)
   avg     44.2    323.5    121.8     49.1     97.0    496.5    133.2    159.4    100.2      2.7    1527.5     986.9+/- 262.4     416.0   explicit_cache[s=4] (this PR)

@YonsiG
Copy link
Contributor Author

YonsiG commented May 1, 2024

This is the timing summary running on cgpu-1 after the current change.
Screenshot 2024-05-01 at 6 16 20 PM
This is the timing summary on cgpu-1 of the master.
Screenshot 2024-05-01 at 5 26 54 PM
The timing reduction of T3 kernels is ~20%. Due to moving radius calculation from T5 to T3, the T5 kernel time is also reduced by ~12%.

@slava77
Copy link
Contributor

slava77 commented May 1, 2024

This is the timing summary running on cgpu-1 after the current change.

please rerun to check that s=8 is just a fluctuation

@SegmentLinking SegmentLinking deleted a comment from github-actions bot May 1, 2024
@YonsiG
Copy link
Contributor Author

YonsiG commented May 1, 2024

This is the timing summary running on cgpu-1 after the current change.

please rerun to check that s=8 is just a fluctuation

Yes, I updated the previous comment by replacing the timing info

SDL/Triplet.h Outdated Show resolved Hide resolved
SDL/Triplet.h Outdated Show resolved Hide resolved
SDL/Triplet.h Outdated Show resolved Hide resolved
SDL/Triplet.h Outdated Show resolved Hide resolved
SDL/Triplet.h Outdated Show resolved Hide resolved
SDL/Triplet.h Outdated Show resolved Hide resolved
SDL/Triplet.h Outdated Show resolved Hide resolved
SDL/Triplet.h Outdated Show resolved Hide resolved
SDL/Triplet.h Outdated Show resolved Hide resolved
@slava77
Copy link
Contributor

slava77 commented May 3, 2024

/run standalone
/run cmssw

Copy link

github-actions bot commented May 3, 2024

The PR was built and ran successfully in standalone mode. Here are some of the comparison plots.

Efficiency vs pT comparison Efficiency vs eta comparison
Fake rate vs pT comparison Fake rate vs eta comparison
Duplicate rate vs pT comparison Duplicate rate vs eta comparison

The full set of validation and comparison plots can be found here.

Here is a timing comparison:

   Evt    Hits       MD       LS      T3       T5       pLS       pT5      pT3      TC       Reset    Event     Short             Rate
   avg     43.1    323.0    124.8     71.1     93.3    544.9    129.6    159.2    105.1      3.1    1597.3    1009.3+/- 266.4     436.8   explicit_cache[s=4] (master)
   avg     45.6    323.4    123.7     50.6     98.5    548.8    135.9    158.9    108.9      2.4    1596.6    1002.2+/- 264.6     433.4   explicit_cache[s=4] (this PR)

SDL/Triplet.h Outdated Show resolved Hide resolved
SDL/Triplet.h Outdated Show resolved Hide resolved
Copy link

github-actions bot commented May 3, 2024

The PR was built and ran successfully with CMSSW. Here are some plots.

OOTB All Tracks
Efficiency and fake rate vs pT, eta, and phi

The full set of validation and comparison plots can be found here.

YonsiG and others added 2 commits May 6, 2024 16:56
SDL/Triplet.h Outdated Show resolved Hide resolved
SDL/Triplet.h Outdated Show resolved Hide resolved
YonsiG and others added 2 commits May 7, 2024 20:14
Co-authored-by: Slava Krutelyov <[email protected]>
Co-authored-by: Slava Krutelyov <[email protected]>
@slava77
Copy link
Contributor

slava77 commented May 7, 2024

/run standalone
/run CMSSW

Copy link

github-actions bot commented May 7, 2024

The PR was built and ran successfully in standalone mode. Here are some of the comparison plots.

Efficiency vs pT comparison Efficiency vs eta comparison
Fake rate vs pT comparison Fake rate vs eta comparison
Duplicate rate vs pT comparison Duplicate rate vs eta comparison

The full set of validation and comparison plots can be found here.

Here is a timing comparison:

   Evt    Hits       MD       LS      T3       T5       pLS       pT5      pT3      TC       Reset    Event     Short             Rate
   avg     43.5    323.7    122.9     70.6     93.7    545.5    131.4    157.0    104.4      3.0    1595.7    1006.7+/- 268.6     435.2   explicit_cache[s=4] (master)
   avg     45.0    323.6    122.1     48.5     97.4    545.1    134.4    157.4    105.7      2.6    1581.9     991.8+/- 267.2     432.5   explicit_cache[s=4] (this PR)

Copy link

github-actions bot commented May 7, 2024

The PR was built and ran successfully with CMSSW. Here are some plots.

OOTB All Tracks
Efficiency and fake rate vs pT, eta, and phi

The full set of validation and comparison plots can be found here.

@slava77 slava77 merged commit 9f96dee into master May 7, 2024
3 checks passed
@YonsiG YonsiG deleted the T3_removeRedundant_new branch May 8, 2024 15:38
@GNiendorf
Copy link
Member

Saw this warning while compiling, is this okay? Warning is maybe-uninitialized, not sure if it was discussed already.
Screenshot 2024-05-10 at 10 30 26 PM

@YonsiG
Copy link
Contributor Author

YonsiG commented May 14, 2024

Saw this warning while compiling, is this okay? Warning is maybe-uninitialized, not sure if it was discussed already. Screenshot 2024-05-10 at 10 30 26 PM

Thank you for spotting this! I did a check and adds a printout just before line 203, and it can print meaningful values for the betaIn variable. And to be saved to the triplet collection, it has to pass through all the requirements, so for a good triplet the betaIn should be calculated and saved. To be safe, I can add initialization to betaIn as 0, as an extra protection for this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rationalize and improve kernels in Triplet.cu What are delta beta in T3s doing?
3 participants