Include 4 Hits Seeds in Phase2 HLT Menu

[AdrianoDee]

PR description:

In SeedGeneratorFromProtoTracksEDProducer by default includeFourthHit = cms.bool(False) so, as it is, in the Phase2 HLT Menu we truncate pixel seeds (tracks) to 3 hits. This PR fixes this.

Backported to 13_1_X for HLT Upgrade in #42821.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42820/36940

• This PR adds an extra 32KB to repository

[cmsbuild]

A new Pull Request was created by @AdrianoDee (Adriano Di Florio) for master.

It involves the following packages:

• HLTrigger/Configuration (hlt)

@cmsbuild, @missirol, @mmusich, @Martin-Grunewald can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @missirol, @silviodonato, @SohamBhattacharya, @rovere this is something you requested to watch as well.
@rappoccio, @antoniovilela, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

[AdrianoDee]

please test

[AdrianoDee]

type tracking

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-984cf5/34815/summary.html
COMMIT: 77571fa
CMSSW: CMSSW_13_3_X_2023-09-18-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/42820/34815/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

• @saumyaphor4252 Update Phase2 MC GT to clean up unused Phase1 pixel conditions #42794

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-984cf5/34815/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-984cf5/34815/git-merge-result

Comparison Summary

Summary:

• You potentially removed 1 lines from the logs
• Reco comparison results: 140 differences found in the comparisons
• DQMHistoTests: Total files compared: 50
• DQMHistoTests: Total histograms compared: 3358044
• DQMHistoTests: Total failures: 1709
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3356313
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
• Checked 214 log files, 167 edm output root files, 50 DQM output files
• TriggerResults: no differences found

[cmsbuild]

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 (and backports should be raised in the release meeting by the corresponding L2)

[slava77]

@AdrianoDee
please add some validation plots for this change.
I recall that this was not a trivial change offline; although the difference was small.

@VourMa tested some of the HLT tracking variants with this change and it looks like there is a large increase in duplicates for the legacy (non-patatrack) pixel track seeding and some more mixed (fakes down and duplicates still generally up) for the patatrack variant. But I'm not sure now I'm looking at the right snapshot of the comparisons.

[mmusich]

@slava77

@VourMa tested some of the HLT tracking variants with this change and it looks like there is a large increase in duplicates for the legacy (non-patatrack) pixel track seeding and some more mixed (fakes down and duplicates still generally up) for the patatrack variant. But I'm not sure now I'm looking at the right snapshot of the comparisons.

can the tracking POG commit to work on the validation part of #39362 in order to makes these changes show up in PR tests?

[mmusich]

hold

• waiting Include 4 Hits Seeds in Phase2 HLT Menu #42820 (comment) to be clarified.

[cmsbuild]

Pull request has been put on hold by @mmusich
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

[rovere]

@slava77

@VourMa tested some of the HLT tracking variants with this change and it looks like there is a large increase in duplicates for the legacy (non-patatrack) pixel track seeding and some more mixed (fakes down and duplicates still generally up) for the patatrack variant. But I'm not sure now I'm looking at the right snapshot of the comparisons.

can the tracking POG commit to work on the validation part of #39362 in order to makes these changes show up in PR tests?

That would be great, indeed.
We can help in the process, of course!

[slava77]

can the tracking POG commit to work on the validation part of #39362 in order to makes these changes show up in PR tests?

would #42783 already be enough? (now I see that it doesn't have comparisons with MC truth)

the PR tests for phase2 include only 10 events with PU50. So, only gross issues would be seen.

[mmusich]

would #42783 already be enough? (now I see that it doesn't have comparisons with MC truth)

No, it's not enough to see changes in efficiency or fake rate w.r.t. TP-s (only w.r.t. offline reco). I actually used #42783 to review at #42820 (comment).

the PR tests for phase2 include only 10 events with PU50. So, only gross issues would be seen.

as #42820 (comment) you speak of large increase in duplicates. Maybe 10 events would be enough to catch that. In any case we need this for the regular phase-2 release validation at HLT in the longer term.

[AdrianoDee]

So apparently adding one extra hit is bad for endcaps. See the MTV. That's completely counterintuitive for me and may indicate something is working weirdly in track building.

Mostly for the duplicates, as Slava was saying:

Leaving here the table for the seeding also.

[slava77]

your link is for the legacy quadruplets, right? (pixel track nlayers has 4 hits)

my guess is that the "used seed" cleaning plays a significant role here

The pixel track duplicate rate is huge

I guess the 3-hit variant in the building can much more easily be masked as used compared to a 4-hit variant.

[AdrianoDee]

Yes, legacy quadruplets On 20 Sep 2023, at 19:53, Slava Krutelyov ***@***.***> wrote:  your link is for the legacy quadruplets, right? (pixel track nlayers has 4 hits) my guess is that the "used seed" cleaning plays a significant role here The pixel track duplicate rate is huge [image]<https://user-images.githubusercontent.com/4676718/269373893-06c53143-5d92-43ce-8997-c1c29b6c743a.png> I guess the 3-hit variant in the building can much more easily be masked as used compared to a 4-hit variant. — Reply to this email directly, view it on GitHub<#42820 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AEA6IGSAPU53Z3DHNHRTZVLX3MUSDANCNFSM6AAAAAA457QTTU>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[mmusich]

@AdrianoDee @cms-sw/tracking-pog-l2 please clarify if the line of updates being proposed in this PR is being followed up from the TRK POG side or if this PR should be closed (same as the backport #42821)

[AdrianoDee]

I would close this PR since for the baseline, as counterintuitive this could be, this update seems not beneficial. Then when we will transition to GPU n-tuplets we will have to make the change.

[mmusich]

I would close this PR since for the baseline,

please go ahead.