L1 matching module for VBF+dijet HLT #41004

portalesHEP · 2023-03-09T08:46:22Z

PR description:

This PR introduces a new matching module for the VBF+dijet trigger that would be implemented in the VBF parking proposal. It based on the already existing L1 VBF jet matching modules that was limited to "3 jet" matching, but here allowing to match 4 to 6 jets.

PR validation:

The module has been implemented and tested with the paths introduced in the menu /users/lportale/VBFparking_13_0_0_pre4/V29.

cmsbuild · 2023-03-09T09:14:16Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41004/34520

This PR adds an extra 12KB to repository

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

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41004/34520/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41004/34520/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2023-03-09T11:12:45Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41004/34526

This PR adds an extra 16KB to repository

cmsbuild · 2023-03-09T11:13:20Z

A new Pull Request was created by @portalesHEP (Louis Portales) for master.

It involves the following packages:

RecoTauTag/HLTProducers (hlt)

@cmsbuild, @missirol, @Martin-Grunewald can you please review it and eventually sign? Thanks.
@silviodonato, @missirol, @azotz, @mbluj 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 · 2023-03-09T21:32:24Z

Will send review comments by the end of the week.

missirol · 2023-03-11T15:24:42Z

@portalesHEP

In my opinion, the new plugin is too similar to the old one (e.g. the configuration parameters are identical).

I think it would be better to extend L1TJetsMatching adding an option to support this new use case, e.g.

selection = cms.string("VBFPlus2CentralJets")

, where the default value of "selection" (name tbd) would correspond to the current behaviour of L1TJetsMatching.

Could you please update the PR accordingly?

missirol · 2023-03-13T15:01:52Z

@portalesHEP

For what I understand from CMSHLT-2702, the latter can be implemented only once (1) the backport of this PR is integrated in CMSSW_13_0_X, (2) a new CMSSW_13_0_X release is built, and (3) said release is parsed in ConfDB to make the new/updated plugin available in ConfDB.

Since this will take several days, this PR needs to converge very soon, if CMSHLT-2702 is targeting the version "V1.0" of the 2023 HLT menu.

portalesHEP · 2023-03-13T15:20:36Z

Thanks for the suggestion @missirol ! The new module has been merged to L1TJetsMatching. It did not seem to me that a dedicated flag was needed, so I did not implement one. The module has been re-tested in the context of the VBF parking studies, with all the versions (TwoJets, ... , SixJets) working as intended. Let me know if you think a flag should still be added to differenciate between the "old" and "new" matchings, I'll try to add it as fast as possible.

cmsbuild · 2023-03-13T15:26:17Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41004/34588

This PR adds an extra 12KB to repository
Found files with invalid states:
- RecoTauTag/HLTProducers/src/L1TPFJetsMatchingVBFPlus2CentralJets.cc:
  - Added: 8ae3b6d
  - Deleted: 54e528f
- RecoTauTag/HLTProducers/interface/L1TJetsMatchingVBFPlus2CentralJets.h:
  - Added: 8ae3b6d
  - Modified: 64337ad
  - Deleted: 54e528f

missirol · 2023-03-13T15:26:03Z

RecoTauTag/HLTProducers/interface/L1TJetsMatching.h

  output = categorise(*pfMatchedJets, pt1Min_, pt2Min_, pt3Min_, mjjMin_);
-  unique_ptr<std::vector<T>> output1(new std::vector<T>(output.first));
-  unique_ptr<std::vector<T>> output2(new std::vector<T>(output.second));
+  unique_ptr<std::vector<T>> output1(new std::vector<T>(std::get<0>(output)));
+  unique_ptr<std::vector<T>> output2(new std::vector<T>(std::get<1>(output)));
+  unique_ptr<std::vector<T>> output3(new std::vector<T>(std::get<2>(output)));
+  unique_ptr<std::vector<T>> output4(new std::vector<T>(std::get<3>(output)));
+  unique_ptr<std::vector<T>> output5(new std::vector<T>(std::get<4>(output)));

  iEvent.put(std::move(output1), "TwoJets");
  iEvent.put(std::move(output2), "ThreeJets");
+  iEvent.put(std::move(output3), "FourJets");
+  iEvent.put(std::move(output4), "FiveJets");
+  iEvent.put(std::move(output5), "SixJets");


This does not follow the suggestion in #41004 (comment).

The idea is to leave the current behaviour of the module unchanged, and add a parameter (e.g. selectionMode) to steer the behaviour of the plugin, along the lines of the following pseudo-code

if (selectionMode_ == "VBF") { // default behaviour output = categoriseVBF(*pfMatchedJets, pt1Min_, pt2Min_, pt3Min_, mjjMin_); unique_ptr<std::vector<T>> output1(new std::vector<T>(output.first)); unique_ptr<std::vector<T>> output2(new std::vector<T>(output.second)); iEvent.put(std::move(output1), "TwoJets"); iEvent.put(std::move(output2), "ThreeJets"); } else if (selectionMode_ == "VBFPlus2CentralJets") { output = categoriseVBFPlus2CentralJets(*pfMatchedJets, pt1Min_, pt2Min_, pt3Min_, mjjMin_); unique_ptr<std::vector<T>> output1(new std::vector<T>(std::get<0>(output))); unique_ptr<std::vector<T>> output2(new std::vector<T>(std::get<1>(output))); unique_ptr<std::vector<T>> output3(new std::vector<T>(std::get<2>(output))); iEvent.put(std::move(output1), "FourJets"); iEvent.put(std::move(output2), "FiveJets"); iEvent.put(std::move(output3), "SixJets"); }

Flag is now introduced, and preserves original functionality when default value is used. Re-tested in the context of VBF parking studies with no apparent changes in the results

cmsbuild · 2023-03-13T15:26:41Z

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

missirol · 2023-03-13T15:32:39Z

Thanks for the suggestion @missirol ! The new module has been merged to L1TJetsMatching. It did not seem to me that a dedicated flag was needed, so I did not implement one. The module has been re-tested in the context of the VBF parking studies, with all the versions (TwoJets, ... , SixJets) working as intended. Let me know if you think a flag should still be added to differenciate between the "old" and "new" matchings, I'll try to add it as fast as possible.

Sorry, I saw this after my latest review comment. It's better to have the flag to keep the current behaviour unchanged; otherwise the modules in the current HLT will do more work than they should (and produce output collections that would not be used). Thanks in advance for addressing the comment.

cmsbuild · 2023-03-14T14:39:01Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41004/34624

This PR adds an extra 12KB to repository

cmsbuild · 2023-03-14T14:39:26Z

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

missirol · 2023-03-14T14:39:39Z

please test

@portalesHEP , looks okay to me. Thanks for addressing comments quickly. Please make sure the latest version of this PR works as intended for your use case (e.g. CMSHLT-2702), since the PR tests won't really test the new functionalities of the plugin.

cmsbuild · 2023-03-14T18:36:33Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2f4c7e/31270/summary.html
COMMIT: 72149dd
CMSSW: CMSSW_13_1_X_2023-03-14-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/41004/31270/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially removed 19 lines from the logs
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3550756
DQMHistoTests: Total failures: 4
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3550730
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 213 log files, 164 edm output root files, 49 DQM output files
TriggerResults: no differences found

missirol · 2023-03-15T07:12:36Z

+hlt

extends an existing producer used at HLT, without changing its default behaviour, and without changing the results of the current Run-3 HLT menu

missirol · 2023-03-15T07:12:54Z

hold

@portalesHEP , please let us know if you verified that the latest version of this PR works as intended.

cmsbuild · 2023-03-15T07:12:58Z

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

portalesHEP · 2023-03-15T12:57:39Z

@portalesHEP , please let us know if you verified that the latest version of this PR works as intended.

Sorry for the delay, I have been having issues with condor this morning. I am not expecting any particular issues from quick interactive tests,and I should be able to confirm with more confidence by the end of the day hopefully

missirol · 2023-03-17T08:40:45Z

@portalesHEP , kind ping on this PR.

perrotta · 2023-03-17T08:55:34Z

RecoTauTag/HLTProducers/interface/L1TJetsMatching.h

+      for (unsigned int j = i + 1; j < pfMatchedJets.size(); j++) {
+        const T& myJet2 = (pfMatchedJets)[j];
+
+        const double mjj_test = (myJet1.p4() + myJet2.p4()).M();


To improve performance, you could replace M() with M2() (even in another cleanup PR):

Suggested change

const double mjj_test = (myJet1.p4() + myJet2.p4()).M();

const double mjj_test = (myJet1.p4() + myJet2.p4()).M();

Suggested change

const double mjj_test = (myJet1.p4() + myJet2.p4()).M();

const double m2jj_test = (myJet1.p4() + myJet2.p4()).M2();

portalesHEP · 2023-03-17T15:04:31Z

again sorry for the delays. I could run some tests and everything seems to run as expected.

missirol · 2023-03-17T17:29:33Z

unhold

I'm taking #41004 (comment) as validation of this PR by the proponents.

cmsbuild · 2023-03-17T17:29:57Z

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

perrotta · 2023-03-18T07:02:48Z

+1

There are not yet implemented suggestions to improve the code, that should be addressed in a follow up PR, i.e.:
- L1 matching module for VBF+dijet HLT #41004 (comment)
- L1 matching module for VBF+dijet HLT #41004 (review)

cmsbuild added this to the CMSSW_13_1_X milestone Mar 9, 2023

cmsbuild added hlt-pending pending-signatures tests-pending orp-pending code-checks-pending labels Mar 9, 2023

portalesHEP mentioned this pull request Mar 9, 2023

L1 matching module for VBF+dijet HLT (13_0_X) #41000

Merged

cmsbuild added code-checks-rejected code-checks-pending and removed code-checks-pending code-checks-rejected labels Mar 9, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 9, 2023

cmsbuild added code-checks-pending and removed code-checks-approved labels Mar 13, 2023

missirol reviewed Mar 13, 2023

View reviewed changes

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 13, 2023

cmsbuild added code-checks-pending and removed code-checks-approved labels Mar 13, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 14, 2023

cmsbuild added tests-started and removed tests-pending labels Mar 14, 2023

cmsbuild added tests-approved and removed tests-started labels Mar 14, 2023

cmsbuild added hlt-approved hold and removed hlt-pending labels Mar 15, 2023

perrotta reviewed Mar 17, 2023

View reviewed changes

cmsbuild added fully-signed and removed pending-signatures hold labels Mar 17, 2023

cmsbuild added orp-approved and removed orp-pending labels Mar 18, 2023

cmsbuild merged commit a162e53 into cms-sw:master Mar 18, 2023

perrotta mentioned this pull request Mar 19, 2023

Reduce non necessary sqrt computations in RecoTauTag/HLTProducers #41100

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

L1 matching module for VBF+dijet HLT #41004

L1 matching module for VBF+dijet HLT #41004

portalesHEP commented Mar 9, 2023

cmsbuild commented Mar 9, 2023

cmsbuild commented Mar 9, 2023

cmsbuild commented Mar 9, 2023

missirol commented Mar 9, 2023

missirol commented Mar 11, 2023

missirol commented Mar 13, 2023

portalesHEP commented Mar 13, 2023

cmsbuild commented Mar 13, 2023

missirol Mar 13, 2023

portalesHEP Mar 13, 2023

cmsbuild commented Mar 13, 2023

missirol commented Mar 13, 2023

cmsbuild commented Mar 14, 2023

cmsbuild commented Mar 14, 2023

missirol commented Mar 14, 2023

cmsbuild commented Mar 14, 2023

missirol commented Mar 15, 2023

missirol commented Mar 15, 2023

cmsbuild commented Mar 15, 2023

portalesHEP commented Mar 15, 2023

missirol commented Mar 17, 2023

perrotta Mar 17, 2023

portalesHEP commented Mar 17, 2023

missirol commented Mar 17, 2023

cmsbuild commented Mar 17, 2023

perrotta commented Mar 18, 2023

	const double mjj_test = (myJet1.p4() + myJet2.p4()).M();
	const double mjj_test = (myJet1.p4() + myJet2.p4()).M();

	const double mjj_test = (myJet1.p4() + myJet2.p4()).M();
	const double m2jj_test = (myJet1.p4() + myJet2.p4()).M2();

L1 matching module for VBF+dijet HLT #41004

L1 matching module for VBF+dijet HLT #41004

Conversation

portalesHEP commented Mar 9, 2023

PR description:

PR validation:

cmsbuild commented Mar 9, 2023

cmsbuild commented Mar 9, 2023

cmsbuild commented Mar 9, 2023

missirol commented Mar 9, 2023

missirol commented Mar 11, 2023

missirol commented Mar 13, 2023

portalesHEP commented Mar 13, 2023

cmsbuild commented Mar 13, 2023

missirol Mar 13, 2023

Choose a reason for hiding this comment

portalesHEP Mar 13, 2023

Choose a reason for hiding this comment

cmsbuild commented Mar 13, 2023

missirol commented Mar 13, 2023

cmsbuild commented Mar 14, 2023

cmsbuild commented Mar 14, 2023

missirol commented Mar 14, 2023

cmsbuild commented Mar 14, 2023

Comparison Summary

missirol commented Mar 15, 2023

missirol commented Mar 15, 2023

cmsbuild commented Mar 15, 2023

portalesHEP commented Mar 15, 2023

missirol commented Mar 17, 2023

perrotta Mar 17, 2023

Choose a reason for hiding this comment

portalesHEP commented Mar 17, 2023

missirol commented Mar 17, 2023

cmsbuild commented Mar 17, 2023

perrotta commented Mar 18, 2023