Format for clustering CSC rechits with jet algorithms #35543

kakwok · 2021-10-05T15:59:36Z

PR description:

This PR adds a new class CSCJetCandidate that builds from CSCRecHit2D and conform to Reco::Candidate format.
The new collection can be used for running jet algorithms on CSC rechits.
This is an essential ingredient for an HLT path for LLP in Run 3.

Presentation at Reco meeting, Oct 1

PR validation:

runTheMatrix.py -l limited -i all --ibeos
31 0 0 0 0 0 0 0 0 tests passed, 8 31 0 0 0 0 0 0 0 failed

cmsbuild · 2021-10-05T16:07:42Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35543/25769

This PR adds an extra 24KB to repository

cmsbuild · 2021-10-05T16:08:07Z

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

It involves the following packages:

DataFormats/CSCRecHit (reconstruction)
RecoLocalMuon/CSCRecHitD (reconstruction)

@jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@HuguesBrun, @calderona, @abbiendi, @Fedespring, @bellan, @sscruz, @jhgoh, @ptcox, @rovere, @trocino, @cericeci this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

slava77

this is just a quick set of comments on the style

slava77 · 2021-10-05T20:53:11Z

RecoLocalMuon/CSCRecHitD/src/CSCJetCandidateProducer.h

+#include "Geometry/CSCGeometry/interface/CSCGeometry.h"
+#include "DataFormats/MuonDetId/interface/CSCDetId.h"
+
+class CSCJetCandidateProducer : public edm::stream::EDProducer<> {


header files are not needed for plugins; please merge this .h file contents into the CSCJetCandidateProducer.cc

all new modules should have a fillDescriptions method defined to provide a default configuration
https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideConfigurationValidationAndHelp

slava77 · 2021-10-05T20:53:40Z

DataFormats/CSCRecHit/BuildFile.xml

-<use name="DataFormats/TrackingRecHit"/>
+<use   name="DataFormats/Common"/>


please revert to the old indentation

slava77 · 2021-10-05T20:54:28Z

DataFormats/CSCRecHit/src/classes_def.xml

+  <class name="reco::CSCJetCandidate" ClassVersion="4">
+    <version ClassVersion="3" checksum="1185432696"/>
+    <version ClassVersion="4" checksum="882507185"/>


Suggested change

<class name="reco::CSCJetCandidate" ClassVersion="4">

<version ClassVersion="3" checksum="1185432696"/>

<version ClassVersion="4" checksum="882507185"/>

<class name="reco::CSCJetCandidate" ClassVersion="3">

<version ClassVersion="3" checksum="882507185"/>

remove intermediate/unused class versions

slava77 · 2021-10-05T20:55:45Z

RecoLocalMuon/CSCRecHitD/src/CSCJetCandidateProducer.cc

+  edm::ESHandle<CSCGeometry> cscG;
+  edm::Handle<CSCRecHit2DCollection> cscRechits;
+
+  setup.get<MuonGeometryRecord>().get(cscG);


esConsumes should be used
https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideHowToGetDataFromES#In_ED_module

slava77 · 2021-10-05T20:58:09Z

DataFormats/CSCRecHit/interface/CSCJetCandidate.h

+#include "DataFormats/Math/interface/Point3D.h"
+#include "DataFormats/Common/interface/SortedCollection.h"
+
+#include <DataFormats/CSCRecHit/interface/CSCRecHit2D.h>


not used.

Suggested change

#include <DataFormats/CSCRecHit/interface/CSCRecHit2D.h>

slava77 · 2021-10-05T21:02:15Z

DataFormats/CSCRecHit/interface/CSCJetCandidate.h

+
+namespace reco {
+
+  class CSCJetCandidate : public LeafCandidate {


I have (so far just minor) preference for such class to be in DataFormats/MuonReco.
Also, the purpose seems to be rather generic and we should probably consider making a more generic type that can handle transparently other muon detectors (DT/RPC/GEM).

slava77 · 2021-10-05T21:05:04Z

DataFormats/CSCRecHit/src/CSCJetCandidate.cc

+    : LeafCandidate(0, LorentzVector(math::PtEtaPhiMLorentzVector(1.0, eta, phi, 0)), Point(0, 0, 0)) {
+  x_ = x;


Suggested change

: LeafCandidate(0, LorentzVector(math::PtEtaPhiMLorentzVector(1.0, eta, phi, 0)), Point(0, 0, 0)) {

x_ = x;

: LeafCandidate(0, LorentzVector(math::PtEtaPhiMLorentzVector(1.0, eta, phi, 0)), Point(0, 0, 0)), x_(x) {

initializer list (before { is preferred to the use of the constructor body)

[the comment applies to the rest of the members]

slava77 · 2021-10-05T21:07:34Z

@cmsbuild please test

cmsbuild · 2021-10-06T01:23:05Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8a69e7/19421/summary.html
COMMIT: f5f3b03
CMSSW: CMSSW_12_1_X_2021-10-05-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35543/19421/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS StaticAnalyzer warnings: There are 1 EventSetupRecord::get warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8a69e7/19421/llvm-analysis/esrget-sa.txt for details.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 40
DQMHistoTests: Total histograms compared: 3219394
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3219366
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 39 files compared)
Checked 169 log files, 37 edm output root files, 40 DQM output files
TriggerResults: no differences found

VinInn · 2021-10-06T12:52:53Z

is all this really needed?
would not be better to directly call FastJet and avoid to create new data objects and copy back and forth info?

slava77 · 2021-10-06T15:27:22Z

is all this really needed? would not be better to directly call FastJet and avoid to create new data objects and copy back and forth info?

@laurenhay
please check this PR and perhaps clarify if there is a (configurable) interface to call FastJet directly without redirecting the edm::Event data via a jet producer?

In the reco meeting discussion on Oct 1 the discussion was suggestive that the persisted products in the standard data tiers (e.g. miniAOD) was not yet planned/intended.

laurenhay · 2021-10-07T16:58:14Z

Pulling in @rappoccio to clarify since he's been in contact with Martin on this. From what I understand it's a no, FastJet can only run on compatible jet objects.

rappoccio · 2021-10-07T17:00:47Z

Hi, @VinInn and @slava77

Generally speaking, no, we can't do this at the moment. We really have to rewrite the data formats for this from scratch if we want to avoid the back-and-forth copy. We are in contact with the fastjet authors to put in the hooks so that we can avoid the copy and just run on flat vectors.

(In fact I put a proposal in to get a postdoc to work on this for HL-LHC but it wasn't deemed important enough so wasn't funded).

This would be an HL-LHC target, we definitely shouldn't hold up this PR.

VinInn · 2021-10-07T17:11:03Z

Do not understand. you have just to copy RecHit position in FastJet jets. clusterize. copy back into Clusters.
you know: as in http://fastjet.fr/repo/doxygen-3.3.4/04-constituents_8cc_source.html
This is infinitely easier that what done here. I am very sorry but I see real problems for all this to be acceptable at HLT.
(even in reco, for what I would be concerned).

ptcox · 2021-10-07T17:24:09Z

Hi Slava, What is all this about? Why is somebody adding code to CSC Local Reco? I have heard nothing about it. Thanks, Tim Slava Krutelyov wrote on 10/6/21 17:27:

…

is all this really needed? would not be better to directly call FastJet and avoid to create new data objects and copy back and forth info? @laurenhay <https://github.com/laurenhay> please check this PR and perhaps clarify if there is a (configurable) interface to call FastJet directly without redirecting the |edm::Event| data via a jet producer? In the reco meeting discussion on Oct 1 the discussion was suggestive that the persisted products in the standard data tiers (e.g. miniAOD) was not yet planned/intended. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#35543 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGYLHQ2PGFMHFWCGLCVZY3UFRTGPANCNFSM5FMA5VOQ>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

rappoccio · 2021-10-07T17:27:39Z

Hi @VinInn we've been doing this at the HLT level since 2009, it's no different from every other jet collection at HLT.

@ptcox this is for an EXO-based trigger from @kakwok for long-lived jets.

ptcox · 2021-10-07T17:32:25Z

Yes, all well and good, but it shouldn't be inside CSC local reco packages! Surely its just a consumer of CSC rechits? Tim rappoccio wrote on 10/7/21 19:27:

…

Hi @VinInn <https://github.com/VinInn> we've been doing this at the HLT level since 2009, it's no different from every other jet collection at HLT. @ptcox <https://github.com/ptcox> this is for an EXO-based trigger from @kakwok <https://github.com/kakwok> for long-lived jets. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#35543 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGYLHWVEITF3FGQFGETTBTUFXKBPANCNFSM5FMA5VOQ>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

slava77 · 2021-10-07T17:32:38Z

Hi Tim,

I thought that this analysis was closely known in the CSC DPG. I guess I've overestimated.
As I commented in the preliminary review, I have (so far just minor) preference for such class to be in DataFormats/MuonReco.

ptcox · 2021-10-07T17:37:36Z

Hi Slava, No I've never heard of it. It's nothing to do with CSC DPG. And I'd really really like it NOT to be entangled into CSC rechit building. I do not want to have to worry about somebody else's tangential code inside CSCRecHitD. Tim Slava Krutelyov wrote on 10/7/21 19:32:

…

Hi Tim, I thought that this analysis was closely known in the CSC DPG. I guess I've overestimated. As I commented in the preliminary review, |I have (so far just minor) preference for such class to be in DataFormats/MuonReco.| — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#35543 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGYLHR6YVMCUDBYN7XQRE3UFXKUDANCNFSM5FMA5VOQ>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

kakwok · 2021-10-07T17:46:58Z

Hi @ptcox , yes, it's a consumer of CSC rechits. We can move the code to DataFormats/MuonReco as suggested by @slava77

Hi @rappoccio, if I understand correctly, @VinInn 's proposal is to write a new producer directly calling fastjet, which plays the role of the fastjetJetProducer to cluster on csc rechit directly. Would that be possible?
We would still need a new data format to hold the cluster information though.

rappoccio · 2021-10-08T11:59:25Z

Hi, @kakwok that is indeed @VinInn's suggestion but I don't see the advantage to having parallel software that does exactly the same thing that is implemented already in cmssw.

dpiparo · 2021-10-08T12:12:27Z

Isn't @VinInn perhaps trying to hint to the costs implied by the approach?

rappoccio · 2021-10-08T12:19:01Z

@dpiparo they're the same as other jet HLT paths. I also don't see the advantage. We still have to copy rechits to a new DataFormat just like we do now, but @VinInn's suggestion has the disadvantage of having parallel code that implements exactly the same thing as FastjetProducer, but with fewer features implemented and less validation ;).

A better approach is to fix this "en masse" during / after Run 3 so the data handling in and out of fastjet is done in a better way. We're already in contact with the authors (along with IRIS-HEP people), and the way forward is relatively clear, we just need someone who can do the implementation.

VinInn · 2021-10-08T12:23:10Z

On 8 Oct, 2021, at 2:19 PM, rappoccio ***@***.***> wrote: We still have to copy rechits to a new DataFormat just like we do now

Why? One needs just to populate locally the FastJet input vector v.

rappoccio · 2021-10-08T12:26:13Z

So that @kakwok and collaborators can access information about the CSC rechits from the jets they're making. It needs to be persistified one way or another.

https://github.com/cms-sw/cmssw/pull/35543/files#diff-e9b281fcb4d161c1ad2090f7ea36ea1f78ce9dc27143496de45e16d6a7fb348aR62-R74

VinInn · 2021-10-08T12:29:03Z

On 8 Oct, 2021, at 2:26 PM, rappoccio ***@***.***> wrote: So that @kakwok and collaborators can access information about the CSC rechits from the jets they're making. It needs to be persistified one way or another. https://github.com/cms-sw/cmssw/pull/35543/files#diff-e9b281fcb4d161c1ad2090f7ea36ea1f78ce9dc27143496de45e16d6a7fb348aR62-R74

what one builds is not Jet as-we-know-it. It's a local cluster (as in all other detectors). One builds a specific CSCCluster with a RefVector to the hits in question. v.

rappoccio · 2021-10-08T12:39:46Z

IMO, "a specific CSC cluster with a RefVector" is a new dataformat that should be encapsulated concisely.

Anyway, you all have the last word. I think it's not going to be faster in the end, it will duplicate code, and will needlessly add to the long-term maintenance and validation costs. My 0.02 CHF. I'll shut up now :)

slava77 · 2021-10-25T13:56:52Z

@kakwok
please clarify on the status of updates to this PR.
Thank you.

kakwok · 2021-10-25T15:22:42Z

@slava77
As discussed in the Reco meeting earlier, I have created a new PR #35836 that implements the direct fastjet approach as suggested by @VinInn

slava77 · 2021-10-25T15:35:33Z

-1

superseded by #35836

perrotta · 2021-11-23T11:38:11Z

@kakwok , since the PR development is going on in #35836, I close this one to clean up the queue.
Please reopen if I misunderstood your intentions

Martin added 9 commits October 4, 2021 18:47

initial implementation

99ccc39

initial implementation [port 10_2_X]

80bda88

clean up implementation [port 10_2_X]

3cb2a02

add class def

bf8a9f8

Fix class_def

99c7b87

add rechit variables

130b698

update classes_def version for CSCJetCandidate

5c04e0b

Update to add CSC rechit info

d4d1286

Update format

f5f3b03

cmsbuild added this to the CMSSW_12_1_X milestone Oct 5, 2021

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Oct 5, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 5, 2021

slava77 reviewed Oct 5, 2021

View reviewed changes

cmsbuild added tests-started and removed tests-pending labels Oct 5, 2021

cmsbuild added tests-approved and removed tests-started labels Oct 6, 2021

kakwok mentioned this pull request Oct 25, 2021

Data Format for hadronic showers in muon system #35836

Merged

cmsbuild added reconstruction-rejected and removed reconstruction-pending labels Oct 25, 2021

smuzaffar modified the milestones: CMSSW_12_1_X, CMSSW_12_2_X Oct 29, 2021

perrotta closed this Nov 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Format for clustering CSC rechits with jet algorithms #35543

Format for clustering CSC rechits with jet algorithms #35543

kakwok commented Oct 5, 2021

cmsbuild commented Oct 5, 2021

cmsbuild commented Oct 5, 2021

slava77 left a comment

slava77 Oct 5, 2021

slava77 Oct 5, 2021

slava77 Oct 5, 2021

slava77 Oct 5, 2021

slava77 Oct 5, 2021

slava77 Oct 5, 2021

slava77 Oct 5, 2021

slava77 commented Oct 5, 2021

cmsbuild commented Oct 6, 2021

VinInn commented Oct 6, 2021

slava77 commented Oct 6, 2021

laurenhay commented Oct 7, 2021

rappoccio commented Oct 7, 2021

VinInn commented Oct 7, 2021 •

edited

ptcox commented Oct 7, 2021 via email

rappoccio commented Oct 7, 2021

ptcox commented Oct 7, 2021 via email

slava77 commented Oct 7, 2021

ptcox commented Oct 7, 2021 via email

kakwok commented Oct 7, 2021 •

edited

rappoccio commented Oct 8, 2021

dpiparo commented Oct 8, 2021

rappoccio commented Oct 8, 2021

VinInn commented Oct 8, 2021 via email

rappoccio commented Oct 8, 2021

VinInn commented Oct 8, 2021 via email

rappoccio commented Oct 8, 2021

slava77 commented Oct 25, 2021

kakwok commented Oct 25, 2021 •

edited

slava77 commented Oct 25, 2021

perrotta commented Nov 23, 2021

		<use name="DataFormats/TrackingRecHit"/>
		<use name="DataFormats/Common"/>


		namespace reco {

		class CSCJetCandidate : public LeafCandidate {

		: LeafCandidate(0, LorentzVector(math::PtEtaPhiMLorentzVector(1.0, eta, phi, 0)), Point(0, 0, 0)) {
		x_ = x;

Format for clustering CSC rechits with jet algorithms #35543

Format for clustering CSC rechits with jet algorithms #35543

Conversation

kakwok commented Oct 5, 2021

PR description:

PR validation:

cmsbuild commented Oct 5, 2021

cmsbuild commented Oct 5, 2021

slava77 left a comment

Choose a reason for hiding this comment

slava77 Oct 5, 2021

Choose a reason for hiding this comment

slava77 Oct 5, 2021

Choose a reason for hiding this comment

slava77 Oct 5, 2021

Choose a reason for hiding this comment

slava77 Oct 5, 2021

Choose a reason for hiding this comment

slava77 Oct 5, 2021

Choose a reason for hiding this comment

slava77 Oct 5, 2021

Choose a reason for hiding this comment

slava77 Oct 5, 2021

Choose a reason for hiding this comment

slava77 commented Oct 5, 2021

cmsbuild commented Oct 6, 2021

Comparison Summary

VinInn commented Oct 6, 2021

slava77 commented Oct 6, 2021

laurenhay commented Oct 7, 2021

rappoccio commented Oct 7, 2021

VinInn commented Oct 7, 2021 • edited

ptcox commented Oct 7, 2021 via email

rappoccio commented Oct 7, 2021

ptcox commented Oct 7, 2021 via email

slava77 commented Oct 7, 2021

ptcox commented Oct 7, 2021 via email

kakwok commented Oct 7, 2021 • edited

rappoccio commented Oct 8, 2021

dpiparo commented Oct 8, 2021

rappoccio commented Oct 8, 2021

VinInn commented Oct 8, 2021 via email

rappoccio commented Oct 8, 2021

VinInn commented Oct 8, 2021 via email

rappoccio commented Oct 8, 2021

slava77 commented Oct 25, 2021

kakwok commented Oct 25, 2021 • edited

slava77 commented Oct 25, 2021

perrotta commented Nov 23, 2021

VinInn commented Oct 7, 2021 •

edited

kakwok commented Oct 7, 2021 •

edited

kakwok commented Oct 25, 2021 •

edited