-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Reduce minYsizeB1 and minYsizeB2 CA cuts for Phase1 #47271
Conversation
cms-bot internal usage |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47271/43578
|
A new Pull Request was created by @bdanzi for master. It involves the following packages:
@Dr15Jones, @bsunanda, @civanch, @cmsbuild, @kpedro88, @makortel, @mdhildreth can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
static constexpr int minYsizeB1 = 36; | ||
static constexpr int minYsizeB2 = 28; | ||
static constexpr int minYsizeB1 = 1; | ||
static constexpr int minYsizeB2 = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could these parameters be exposed as configurable parameters instead of hardcoded?
I guess this would ease the checks to be asked to various stakeholders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be done in ef9d50d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
type tracking |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47271/43582
|
Pull request #47271 was updated. @Dr15Jones, @bsunanda, @civanch, @cmsbuild, @jfernan2, @kpedro88, @makortel, @mandrenguyen, @mdhildreth can you please check and sign again. |
@cmsbuild, please test
|
@@ -197,6 +199,9 @@ void CAHitNtupletGeneratorOnGPU<pixelTopology::Phase1>::fillDescriptions(edm::Pa | |||
trackQualityCuts.add<double>("quadrupletMaxTip", 0.5)->setComment("Max |Tip| for quadruplets, in cm"); | |||
trackQualityCuts.add<double>("quadrupletMaxZip", 12.)->setComment("Max |Zip| for quadruplets, in cm"); | |||
|
|||
desc.add<int>("minYsizeB1", 1)->setComment("Min Y cluster size in pixel B1"); | |||
desc.add<int>("minYsizeB2", 1)->setComment("Min Y cluster size in pixel B2"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cms-sw/orp-l2, it would be great to get this PR into CMSSW_15_0_0 for 2025 HLT development purposes.
As it is, it does change (HLT) physics, could it enter in pre3 still?
Otherwise I guess it would be more acceptable if it would just make the parameters configurable, followed by a change in the HLT menu configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try to get it merged tomorrow, that will be fine for pre3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excellent, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like for some reason the commands to enable gpu
and test
are not being received correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It now seems to have worked, although tests are still pending - I see a similar behavior in other PRs
+1 Size: This PR adds an extra 40KB to repository Comparison SummarySummary:
|
+1 Size: This PR adds an extra 40KB to repository Comparison SummarySummary:
GPU Comparison SummarySummary:
|
As it was a general issue, I would have hoped it would be taken into account by @cms-sw/orp-l2, especially after the comment by @mandrenguyen, as this would have been fully tested and signed if it wasn't for the issue described above by @smuzaffar. |
We did take the request into account, as you just noted. We decided, however, not to further the delay the pre-release which was scheduled for Tuesday. The final 15_0_0 will not be cut until the 24th. This will be included. |
Thanks. As far as I can tell the main inconvenience is that if this would have entered in pre3 we could have profited of the regular HLT release validation (one pre-release earlier). |
+1 |
@cms-sw/geometry-l2 can you please take a look to this PR and the corresponding backport #47296 ? We'll need those for the 2025 HLT menu development. |
+1 |
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. @rappoccio, @antoniovilela, @sextonkennedy, @mandrenguyen (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
The PR aims to minimize the Cluster Size Cuts for the Phase 1 geometry on the B1 and B2 layers, specifically when considering B1-FPIX and B2-FPIX doublets. These doublets serve as the initial inputs for the CA. The changes have been tested on top of the Phase 1 CA-tuned parameters.
This adjustment results in about 5% relative efficiency gain in
hltIter0PFlowTrackSelectionHighPurity
and a 3% gain inhltMerged
. Additionally, it reduces the reliance on the Doublet Recovery iteration, leading to a corresponding decrease in efficiency forhltDoubletRecoveryPFlowTrackSelectionHighPurity
. Track resolution shows a small improvement as a function of pT.PR validation:
600 events TTBarPU 2025 (/RelValTTbar_14TeV/CMSSW_14_2_1-PU_142X_mcRun3_2025_realistic_v4_STD_Winter25_PU-v1/GEN-SIM-DIGI-RAW):
http://uaf-3.t2.ucsd.edu/~bdanzi/PR_2025/plots_hlt.html
500 events TTBarPU EEOR3 (/RelValTTbar_14TeV/CMSSW_14_0_9-PU_140X_mcRun3_2024_realistic_EOR3_TkDPGv7_RV245_2024-v2/GEN-SIM-DIGI-RAW):
http://uaf-3.t2.ucsd.edu/~bdanzi/PRClusterSize_EEOR3_TTBarPU/plots_hlt.html
Instructions on how to run
Tested in
CMSSW_15_0_0_pre2
at HLT step.Generate the
.cff
file on lxplus and place it insideHLTrigger/Configuration/python
by running:hltGetConfiguration /dev/CMSSW_14_2_0/GRun/V10 --globaltag 140X_mcRun3_2024_realistic_EOR3_TkDPGv7 --mc --unprescale --output minimal --max-events 500 --input file:input_file.root --cff --eras Run3_2024 --l1-emulator uGT --l1 L1Menu_Collisions2024_v1_3_0_xml --paths MC_ReducedIterativeTracking_v22 > hltMC_default_onlyTracking.py
The
.cff
should be loaded to run the following script: