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

Add ticl_v5 modifier at HLT #45500

Merged
merged 3 commits into from
Jul 25, 2024
Merged

Add ticl_v5 modifier at HLT #45500

merged 3 commits into from
Jul 25, 2024

Conversation

AuroraPerego
Copy link
Contributor

PR description:

This PR extends the existing ticl_v5 modifier to the Phase-2 HLT. The default for now remains TICLv4.
In all the .203 wfs the --procModifiers ticl_v5 option is added automatically.
The changes follow the logic of the offline workflow, but with the MTD timing disabled by default.

PR validation:

Checked on wf 29688.203.
A workflow .203 (e.g. 29688.203 and 29888.203) with the option -w upgrade should be used to test this PR.

FYI @waredjeb

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 18, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45500/40966

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @AuroraPerego for master.

It involves the following packages:

  • Configuration/PyReleaseValidation (pdmv, upgrade)
  • HLTrigger/Configuration (hlt)
  • RecoHGCal/TICL (reconstruction, upgrade)

@AdrianoDee, @Martin-Grunewald, @cmsbuild, @jfernan2, @kskovpen, @mandrenguyen, @miquork, @mmusich, @srimanob, @subirsarkar, @sunilUIET can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @SohamBhattacharya, @VourMa, @apsallid, @fabiocos, @felicepantaleo, @forthommel, @hatakeyamak, @lecriste, @makortel, @missirol, @mmusich, @rovere, @sameasy, @silviodonato, @slomeo, @sobhatta, @youyingli this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

@felicepantaleo
Copy link
Contributor

test parameters:

  • workflow_opts= -w upgrade
  • workflow = 29888.203,29688.203,29846.204,29846.205,29891.203,29891.204,29891.205

@felicepantaleo
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45500/40968

@cmsbuild
Copy link
Contributor

Pull request #45500 was updated. @AdrianoDee, @Martin-Grunewald, @cmsbuild, @jfernan2, @kskovpen, @mandrenguyen, @miquork, @mmusich, @srimanob, @subirsarkar, @sunilUIET can you please check and sign again.

@mmusich
Copy link
Contributor

mmusich commented Jul 23, 2024

@AuroraPerego @felicepantaleo overall I don't have further comments on the naming etc.
I just notice that we have a drop of efficiency in the TriggerResults: for .203 workflows

      Events    Accepted      Gained        Lost       Other  Trigger
          10           0           -           -          ~1  HLT_DoubleMediumChargedIsoPFTauHPS40_eta2p1
          10           0           -           -          ~1  HLT_DoubleMediumDeepTauPFTauHPS35_eta2p1
          10          10           -           -         ~10  MC_JME
          10          10           -           -         ~10  MC_BTV
          10           2           -          -2           -  MC_Ele5_Open_Unseeded
          10           2           -          -2           -  MC_Ele5_Open_L1Seeded

Can you comment if you expect loosing events in the MC_Ele5_Open* paths?

@felicepantaleo
Copy link
Contributor

@mmusich it is expected, as the pidcut is disabled here (hopefully the link works, I'm from the phone):
https://github.com/cms-sw/cmssw/pull/45500/files?file-filters%5B%5D=.py&show-viewed-files=true#diff-603a57a6960d020c329208a6fbcf0d0b267a03af039ccbebccc447743a58e697

it will be reenabled in another PR in which we update the pid model

@mmusich
Copy link
Contributor

mmusich commented Jul 24, 2024

+hlt

@mandrenguyen
Copy link
Contributor

I was surprised to see such a big change in the Particle ID here
Is that expected?

@AuroraPerego
Copy link
Contributor Author

I was surprised to see such a big change in the Particle ID here Is that expected?

Yes, part of the changes in TICLv5 affects the linking in the calorimeter and with the track. Both have been improved, so now we are creating fewer PF candidates, particularly less neutral ones.

@mmusich
Copy link
Contributor

mmusich commented Jul 24, 2024

upon second look, there is some collection which is empty now:

image

or reduced in content:

image

@mmusich
Copy link
Contributor

mmusich commented Jul 24, 2024

Both have been improved, so now we are creating fewer PF candidates, particularly less neutral ones.

OK so I guess this explains the reduction in reco::PFCandidates. What about ticlTrackstersMerge?

@AuroraPerego
Copy link
Contributor Author

OK so I guess this explains the reduction in reco::PFCandidates. What about ticlTrackstersMerge?

They do not exist anymore in TICLv5 because they have been replaced with ticlCandiates.

@mandrenguyen
Copy link
Contributor

+reconstruction
Changes expected, as explained in comment

@AdrianoDee
Copy link
Contributor

+pdmv

@mandrenguyen
Copy link
Contributor

AuroraPerego can you please resolve addressed open conversations while we wait for @cms-sw/upgrade-l2 to review/sign?

@AuroraPerego
Copy link
Contributor Author

AuroraPerego can you please resolve addressed open conversations while we wait for @cms-sw/upgrade-l2 to review/sign?

done :)

@srimanob
Copy link
Contributor

+Upgrade

@cmsbuild
Copy link
Contributor

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. @mandrenguyen, @sextonkennedy, @antoniovilela, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

@mandrenguyen
Copy link
Contributor

+1

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.

7 participants