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

Added PFDiJetCorrCheckerWithDiTau module to remove overlap between di-tau and di-jet for HLT #158

Merged

Conversation

namppl
Copy link

@namppl namppl commented Feb 24, 2022

We would like to introduce a new EDProducer "PFJetsTauCorrelationCondition" which will be used in VBF+2tau triggers [1]. The function of this module is quite similar to the recent PR for 2tau+1jet triggers [2]. The module looks for a combination of (j1, j2, t1, t2) (j1, j2 from PFJet, t1,t2 from the tau collection) that there is no matching between j1,j2 and t1,t2, i.e. they are 4 independent objects, and store j1 and j2.

[1] https://its.cern.ch/jira/browse/CMSHLT-2224
[2] cms-sw#36828

@namppl namppl changed the title first commit Added PFJetsTauCorrelationCondition Feb 24, 2022
Copy link

@mbluj mbluj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dear proponents,
thank you for this development.

You can find my detailed comments inline. What concerns generalities:

  • Could you try to change name of the module to more descriptive, please? This module is a producer of a collection of PFJets now overlapping with pairs of taus and forming pairs with a minimal invariant mass - could it be somehow codded in the name of the module?
  • The module produces PFJets, but has "hidden" requirement that there are also at least two (filtered) PFTaus in the event - is it what you want? If so it is indeed not only producer, but also correlation condition checker. Anyway more descriptive name could be usefull.
  • Please add description what exactly the module does in its header file
  • Is there any reason to use global::EDProducer<> instead of stream::EDProducer<>
  • After implementing requested code changes please run scram b code-fromat (you can do it also before)
  • Remove unnecessary include files, includes not needed in the header file move to the implementation file

@namppl
Copy link
Author

namppl commented Feb 28, 2022

Dear Michal,

Many thanks for the detailed comments and sorry for the late response. I was not available last Friday. Your comments have been all implemented.

Best regards,
Kyungwook

@namppl
Copy link
Author

namppl commented Feb 28, 2022

  • Could you try to change name of the module to more descriptive, please? This module is a producer of a collection of PFJets now overlapping with pairs of taus and forming pairs with a minimal invariant mass - could it be somehow codded in the name of the module?

I would change the name to "PFDiJetCorrCheckerWithDiTau"

  • The module produces PFJets, but has "hidden" requirement that there are also at least two (filtered) PFTaus in the event - is it what you want? If so it is indeed not only producer, but also correlation condition checker. Anyway more descriptive name could be usefull.

Yes, the module makes sense only if there are at least 2 jets and 2 taus. "DiJet" and "DiTau" in the new name hopefully clarify that point.

  • Please add description what exactly the module does in its header file

I'll add it asap.

  • Is there any reason to use global::EDProducer<> instead of stream::EDProducer<>

No, the module processes event by event so it should be fine with stream. It's now stream::EDProducer.

  • After implementing requested code changes please run scram b code-fromat (you can do it also before)

Done

  • Remove unnecessary include files, includes not needed in the header file move to the implementation file

Done

@mbluj
Copy link

mbluj commented Feb 28, 2022

Thank you for implementing my changes. The missing thing is a short description (in header file) what the module does. I also spotted that name of configuration argument in l.80 has not been updated -please fix it too.
Finally, I would like to ask you to provide me a recipe (a cfg file) to run the module - I would like to check that it runs w/o failing, but of course I do not pretend to check if it works correctly.

@mbluj mbluj changed the title Added PFJetsTauCorrelationCondition Added PFDiJetCorrCheckerWithDiTau module to remove overlap between di-tau and di-jet for HLT Feb 28, 2022
@namppl
Copy link
Author

namppl commented Feb 28, 2022

I've fixed L80 and added a description in the header file. I can get my colleague to provide a working version of HLT configuration but it will require some tedious works (merge PRs and tweaks on L1) to run the configuration. We've tested the code works correctly last week and will double-check the updated code also works fine.

@mbluj
Copy link

mbluj commented Feb 28, 2022

I've fixed L80 and added a description in the header file. I can get my colleague to provide a working version of HLT configuration but it will require some tedious works (merge PRs and tweaks on L1) to run the configuration. We've tested the code works correctly last week and will double-check the updated code also works fine.

Thank you.

Concerning the test configuration: I think you need dump HLT path from ConfDB to cfg, edit it manually to add the new module and provide me the cfg with instruction how to use it. It should be done to be sure that nothing is messed up, but it does not need any very realistic test scenario.

@mbluj
Copy link

mbluj commented Mar 1, 2022

For records: test configuration was delivered and tests with it are successful => this PR is going to be merged.

@mbluj mbluj merged commit 993cac2 into cms-tau-pog:CMSSW_12_3_X_tau-pog_HLT2Jet2Tau Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants