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.

Sign up for GitHub

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

Jump to bottom

Add upper cut on jet time to HLTJetTimingFilter #41089

Closed
wants to merge 0 commits into from
Closed

Add upper cut on jet time to HLTJetTimingFilter #41089

wants to merge 0 commits into from

Conversation

ssantpur
Copy link

PR description:

This PR allows for more flexibility in choosing the timing thresholds for the delayed jet and delayed E/gamma triggers, this does not effect any existing triggers or their rates. The modification is to enable triggering on the negative timing region that can target beam halo and satellite collisions, and allow for maximum timing to be specified in the positive timing side to enable some delayed jet triggers to be added to parking dataset.

PR validation:

I ran over HLTPhysics using Run2022G dataset and saw no change in the rates.

@cmsbuild cmsbuild added this to the CMSSW_13_1_X milestone Mar 17, 2023
@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41089/34702

  • This PR adds an extra 20KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41089/34705

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • HLTrigger/Egamma (hlt)
  • HLTrigger/JetMET (hlt)

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

cms-bot commands are listed here

@missirol
Copy link
Contributor

@ssantpur

For HLTJetTimingFilter, I can see that an upper cut on jet-time is missing and could be added.

But, in general, I don't see why it's necessary to allow for 2 time windows in the same instance of the filter. Couldn't one simply have two instances of the filter (one per time window), and combine the resulting jet/egamma candidates later on if needed ?

@ssantpur
Copy link
Author

ssantpur commented Mar 17, 2023 •
edited
Loading

@missirol
The upper cut on jet-time is added as jetMaxTimeThresh_ on the positive timing side (with a default value of 12.5ns) and as jetMaxNegTimeThresh_ on the negative timing side (with a default value of -12.5ns). The reason for modifying the existing filter rather than duplicating it is that we would like to allow for a single filter to be used for all the delayed jet applications. This enables us to modify or implement the HLT paths downstream more easily.

Please let me know if this is a problem and I can have two instances of the same filter instead.

@missirol
Copy link
Contributor

I think it's worth exploring the option of having 2 instances of the filter, before we complicate the logic of the filters themselves.

Feel free to share the corresponding HLT configuration, so we can judge what is the cleaner option.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41089/34825

  • This PR adds an extra 8KB to repository

@cmsbuild
Copy link
Contributor

Pull request #41089 was updated. @cmsbuild, @missirol, @Martin-Grunewald can you please check and sign again.

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41089/34827

  • This PR adds an extra 24KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@ssantpur
Copy link
Author

@missirol
I updated the branch as instructed and I agree that jetTimeThresh can be moved to jetMinTimeThresh in the future. It is not done here to allow for other HLT paths to continue using the jet timing filter without needing any modifications for v1.0. We can revisit it in the future. Let me know if anything else needs to be done.

@missirol
Copy link
Contributor

missirol commented Mar 24, 2023 •
edited
Loading

I updated the branch as instructed

I doubt that. As you can see, now the branch contains unrelated changes. Please, follow #41089 (comment). Otherwise, close this PR, and open a new PR with a single commit.

@missirol
Copy link
Contributor

missirol commented Mar 24, 2023 •
edited
Loading

I doubt that. As you can see, now the branch contains unrelated changes. Please, follow #41089 (comment). Otherwise, close this PR, and open a new PR with a single commit.

Scratch this. There was a mistake in my previous comment. You can rebase on top of CMSSW_13_1_0_pre2 (instead of 13_0_1).

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41089/34858

@cmsbuild
Copy link
Contributor

Pull request #41089 was updated. @SiewYan, @rappoccio, @kskovpen, @bbilin, @pmandrik, @Saptaparna, @Martin-Grunewald, @alberto-sanchez, @menglu21, @cecilecaillol, @GurpreetSinghChahal, @perrotta, @civanch, @vlimant, @makortel, @cmsbuild, @missirol, @fwyzard, @simonepigazzini, @mandrenguyen, @davidlange6, @smuzaffar, @Dr15Jones, @epalencia, @emanueleusai, @mdhildreth, @AdrianoDee, @syuvivida, @micsucmed, @fabiocos, @francescobrivio, @mkirsano, @swertz, @bsunanda, @clacaputo, @srimanob, @saumyaphor4252, @aloeliger, @sunilUIET, @tvami, @rvenditti can you please check and sign again.

@ssantpur ssantpur closed this Mar 24, 2023
@ssantpur ssantpur force-pushed the TwoSidedTimingFilter branch from f43f616 to 1cc25cf Compare March 24, 2023 14:56
This was referenced Mar 24, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@missirol missirol missirol left review comments

Assignees
No one assigned
Labels
alca-pending analysis-pending code-checks-approved core-pending db-pending dqm-pending generators-pending geometry-pending heterogeneous-pending hlt-pending l1-pending operations-pending orp-pending pdmv-pending pending-signatures reconstruction-pending simulation-pending tests-pending tracking trk upgrade-pending xpog-pending
Projects
None yet
Milestone
CMSSW_13_1_X
Development

Successfully merging this pull request may close these issues.
3 participants
@ssantpur @cmsbuild @missirol