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

pT dependent ROI producers for Muon HLT #37286

Merged
merged 3 commits into from Apr 5, 2022

Conversation

khaosmos93
Copy link
Contributor

This PR adds two new tracking region producers with pt parametrized ROI sizes for the inside-out track reconstruction in Muon HLT. These modules are minimally changed from the existing modules:

  • RecoMuon/GlobalTrackingTools/interface/MuonTrackingRegionByPtBuilder.h is updated from RecoMuon/GlobalTrackingTools/interface/MuonTrackingRegionBuilder.h
  • RecoTracker/TkTrackingRegions/plugins/L1MuonSeededTrackingRegionsProducer.h is updated from RecoTracker/TkTrackingRegions/plugins/CandidateSeededTrackingRegionsProducer.h

Initial performance study was presented at the TSG meeting by @wonpoint4: https://indico.cern.ch/event/1131517/contributions/4751084/attachments/2396360/4097442/220223_TSG_MuonROI_wonjun.pdf

  • Significant efficiency gain at low pT, e.g. 75% to 95% at 4 GeV, with a cost of up to around 10 ms timing increase and O(10%) of rate increase for low-pT muon triggers (no significant impact on the total rates).
  • The final ROI parameter tuning (python configuration level changes) to balance between efficiency and timing/rates is ongoing.

FYI: @JanFSchulte @wonpoint4

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37286/28914

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @khaosmos93 (Minseok Oh) for master.

It involves the following packages:

  • RecoMuon/GlobalTrackingTools (reconstruction)
  • RecoTracker/TkTrackingRegions (reconstruction)

@jpata, @cmsbuild, @clacaputo, @slava77 can you please review it and eventually sign? Thanks.
@felicepantaleo, @abbiendi, @CeliaFernandez, @mmusich, @cericeci, @JanFSchulte, @jhgoh, @dgulhan, @sscruz, @trocino, @rociovilar, @GiacomoSguazzoni, @rovere, @VinInn, @bellan, @ebrondol, @mtosi, @amagitte, @HuguesBrun, @Fedespring, @calderona, @gpetruc 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

@missirol
Copy link
Contributor

Hi @khaosmos93 , one question from HLT: would this update eventually be needed in HLT menus for 12_3_X? If so, do you plan to backport this PR to 12_3_X in time for 12_3_0?

@JanFSchulte
Copy link
Contributor

Hi @missirol, the answer is yes, we would need this backported to 12_3_X.

@missirol
Copy link
Contributor

Hi @missirol, the answer is yes, we would need this backported to 12_3_X.

Okay, good to know, will keep an eye on this (cc: @Martin-Grunewald , @silviodonato).

explicit MuonTrackingRegionByPtBuilder(const edm::ParameterSet& par, edm::ConsumesCollector&& iC) { build(par, iC); }

/// Destructor
~MuonTrackingRegionByPtBuilder() override {}
Copy link
Contributor

Choose a reason for hiding this comment

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

could be default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

iC.consumes<MeasurementTrackerEvent>(regPSet.getParameter<edm::InputTag>("measurementTrackerName"));
}
m_searchOpt = false;
if (regPSet.exists("searchOpt"))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this practice is discouraged: #36261

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed.

if (!valid_charge)
charge = 0;

int link = 36 + (int)(it->tfMuonIndex() / 3.);
Copy link
Contributor

Choose a reason for hiding this comment

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

magic numbers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

move to static constexpr


int link = 36 + (int)(it->tfMuonIndex() / 3.);
bool barrel = true;
if ((link >= 36 && link <= 41) || (link >= 66 && link <= 71))
Copy link
Contributor

Choose a reason for hiding this comment

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

more magic numbers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

move to static constexpr

GlobalTrajectoryParameters param(pos, mom, charge, &*service_->magneticField());

AlgebraicSymMatrix55 mat;
mat[0][0] = (0.25 / pt) * (0.25 / pt); // sigma^2(charge/abs_momentum)
Copy link
Contributor

Choose a reason for hiding this comment

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

some more magic numbers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

move to static constexpr

Comment on lines 240 to 242
edm::Handle<MeasurementTrackerEvent> hmte;
iEvent.getByToken(token_measurementTracker, hmte);
measurementTracker = hmte.product();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
edm::Handle<MeasurementTrackerEvent> hmte;
iEvent.getByToken(token_measurementTracker, hmte);
measurementTracker = hmte.product();
measurementTracker = &iEvent.get(token_measurementTracker);

I think this whole section could be simplified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

if (pt < l1MinPt_ || std::abs(eta) > l1MaxEta_)
continue;

float theta = 2 * atan(exp(-eta));
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this very expensive in a double loop? (note that I don't have a good suggestion to overcome this as you need it also to construct the momentum vector below - given the dataformat - , but maybe some of the reviewers have)

bool m_precise;
edm::EDGetTokenT<MeasurementTrackerEvent> token_measurementTracker;
RectangularEtaPhiTrackingRegion::UseMeasurementTracker m_whereToUseMeasurementTracker;
edm::ESGetToken<MagneticField, IdealMagneticFieldRecord> token_field;
Copy link
Contributor

Choose a reason for hiding this comment

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

could be const

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

pt = minPtBarrel_;
} else {
// ME2
theid = theta < Geom::pi() / 2. ? CSCDetId(1, 2, 0, 0, 0) : CSCDetId(2, 2, 0, 0, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason why it can't be M_PI ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

// Flag to use precise??
thePrecise = par.getParameter<bool>("precise");

// perigee reference point ToDo: Check this
Copy link
Contributor

Choose a reason for hiding this comment

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

What does ToDo: Check this means here? Do you have some issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sry, it's something I forgot to remove from the old code.


/// Add Fill Descriptions
static void fillDescriptions(edm::ConfigurationDescriptions& descriptions);
static void fillDescriptionsHLT(edm::ParameterSetDescription& descriptions);
Copy link
Contributor

Choose a reason for hiding this comment

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

if you are not planning to do something like this

void MuonTrackingRegionBuilder::fillDescriptions(edm::ConfigurationDescriptions& descriptions) {
{
edm::ParameterSetDescription desc;
fillDescriptionsOffline(desc);
descriptions.add("MuonTrackingRegionBuilder", desc);
}
{
edm::ParameterSetDescription desc;
fillDescriptionsHLT(desc);
descriptions.add("MuonTrackingRegionBuilderHLT", desc);
}
descriptions.setComment(
"Build a TrackingRegion around a standalone muon. Options to define region around beamspot or primary vertex and "
"dynamic regions are included.");
}
void MuonTrackingRegionBuilder::fillDescriptionsHLT(edm::ParameterSetDescription& desc) {
desc.add<double>("EtaR_UpperLimit_Par1", 0.25);
desc.add<double>("DeltaR", 0.2);
desc.add<edm::InputTag>("beamSpot", edm::InputTag("hltOnlineBeamSpot"));
desc.add<int>("OnDemand", -1);
desc.add<edm::InputTag>("vertexCollection", edm::InputTag("pixelVertices"));
desc.add<double>("Rescale_phi", 3.0);
desc.add<bool>("Eta_fixed", false);
desc.add<double>("Rescale_eta", 3.0);
desc.add<double>("PhiR_UpperLimit_Par2", 0.2);
desc.add<double>("Eta_min", 0.05);
desc.add<bool>("Phi_fixed", false);
desc.add<double>("Phi_min", 0.05);
desc.add<double>("PhiR_UpperLimit_Par1", 0.6);
desc.add<double>("EtaR_UpperLimit_Par2", 0.15);
desc.add<edm::InputTag>("MeasurementTrackerName", edm::InputTag("hltESPMeasurementTracker"));
desc.add<bool>("UseVertex", false);
desc.add<double>("Rescale_Dz", 3.0);
desc.add<bool>("Pt_fixed", false);
desc.add<bool>("Z_fixed", true);
desc.add<double>("Pt_min", 1.5);
desc.add<double>("DeltaZ", 15.9);
desc.add<double>("DeltaEta", 0.2);
desc.add<double>("DeltaPhi", 0.2);
desc.add<int>("maxRegions", 1);
desc.add<bool>("precise", true);
desc.add<edm::InputTag>("input", edm::InputTag("hltL2Muons", "UpdatedAtVtx"));
}
void MuonTrackingRegionBuilder::fillDescriptionsOffline(edm::ParameterSetDescription& desc) {
desc.add<double>("EtaR_UpperLimit_Par1", 0.25);
desc.add<double>("DeltaR", 0.2);
desc.add<edm::InputTag>("beamSpot", edm::InputTag(""));
desc.add<int>("OnDemand", -1);
desc.add<edm::InputTag>("vertexCollection", edm::InputTag(""));
desc.add<double>("Rescale_phi", 3.0);
desc.add<bool>("Eta_fixed", false);
desc.add<double>("Rescale_eta", 3.0);
desc.add<double>("PhiR_UpperLimit_Par2", 0.2);
desc.add<double>("Eta_min", 0.05);
desc.add<bool>("Phi_fixed", false);
desc.add<double>("Phi_min", 0.05);
desc.add<double>("PhiR_UpperLimit_Par1", 0.6);
desc.add<double>("EtaR_UpperLimit_Par2", 0.15);
desc.add<edm::InputTag>("MeasurementTrackerName", edm::InputTag(""));
desc.add<bool>("UseVertex", false);
desc.add<double>("Rescale_Dz", 3.0);
desc.add<bool>("Pt_fixed", false);
desc.add<bool>("Z_fixed", true);
desc.add<double>("Pt_min", 1.5);
desc.add<double>("DeltaZ", 15.9);
desc.add<double>("DeltaEta", 0.2);
desc.add<double>("DeltaPhi", 0.2);
desc.add<int>("maxRegions", 1);
desc.add<bool>("precise", true);
desc.add<edm::InputTag>("input", edm::InputTag(""));
}
, you can remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

/// Add Fill Descriptions
static void fillDescriptions(edm::ConfigurationDescriptions& descriptions);
static void fillDescriptionsHLT(edm::ParameterSetDescription& descriptions);
static void fillDescriptionsOffline(edm::ParameterSetDescription& descriptions);
Copy link
Contributor

Choose a reason for hiding this comment

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

As above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -0,0 +1,86 @@
#ifndef RecoMuon_TrackingTools_MuonTrackingRegionByPtBuilder_H
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#ifndef RecoMuon_TrackingTools_MuonTrackingRegionByPtBuilder_H
#ifndef RecoMuon_GlobalTrackingTools_MuonTrackingRegionByPtBuilder_h

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -0,0 +1,411 @@
#ifndef L1MuonSeededTrackingRegionsProducer_h
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#ifndef L1MuonSeededTrackingRegionsProducer_h
#ifndef RecoTracker_TkTrackingRegions_L1MuonSeededTrackingRegionsProducer_h

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -0,0 +1,411 @@
#ifndef L1MuonSeededTrackingRegionsProducer_h
#define L1MuonSeededTrackingRegionsProducer_h
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#define L1MuonSeededTrackingRegionsProducer_h
#define RecoTracker_TkTrackingRegions_L1MuonSeededTrackingRegionsProducer_h

@@ -0,0 +1,86 @@
#ifndef RecoMuon_TrackingTools_MuonTrackingRegionByPtBuilder_H
#define RecoMuon_TrackingTools_MuonTrackingRegionByPtBuilder_H
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#define RecoMuon_TrackingTools_MuonTrackingRegionByPtBuilder_H
#define RecoMuon_GlobalTrackingTools_MuonTrackingRegionByPtBuilder_h

@clacaputo
Copy link
Contributor

This PR adds two new tracking region producers with pt parametrized ROI sizes for the inside-out track reconstruction in Muon HLT. These modules are minimally changed from the existing modules:

  • RecoMuon/GlobalTrackingTools/interface/MuonTrackingRegionByPtBuilder.h is updated from RecoMuon/GlobalTrackingTools/interface/MuonTrackingRegionBuilder.h
  • RecoTracker/TkTrackingRegions/plugins/L1MuonSeededTrackingRegionsProducer.h is updated from RecoTracker/TkTrackingRegions/plugins/CandidateSeededTrackingRegionsProducer.h

Initial performance study was presented at the TSG meeting by @wonpoint4: https://indico.cern.ch/event/1131517/contributions/4751084/attachments/2396360/4097442/220223_TSG_MuonROI_wonjun.pdf

  • Significant efficiency gain at low pT, e.g. 75% to 95% at 4 GeV, with a cost of up to around 10 ms timing increase and O(10%) of rate increase for low-pT muon triggers (no significant impact on the total rates).
  • The final ROI parameter tuning (python configuration level changes) to balance between efficiency and timing/rates is ongoing.

FYI: @JanFSchulte @wonpoint4

Hi @khaosmos93 , on which workflow did you test it?

@clacaputo
Copy link
Contributor

@cmsbuild please test

@JanFSchulte
Copy link
Contributor

Just fyi, Minseok is moving to Europe currently, so questions and comments will be addressed next week.

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-60bbc1/23395/summary.html
COMMIT: f2bf00c
CMSSW: CMSSW_12_4_X_2022-03-25-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37286/23395/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 3501 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3695650
  • DQMHistoTests: Total failures: 51917
  • DQMHistoTests: Total nulls: 59
  • DQMHistoTests: Total successes: 3643652
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 204 log files, 45 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37286/29107

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

Pull request #37286 was updated. @jpata, @cmsbuild, @clacaputo, @slava77 can you please check and sign again.

@khaosmos93
Copy link
Contributor Author

Thank you very much for your comments, @mmusich, @clacaputo! I tried to address most of them.
These modules are used only in the HLT and therefore tested with the latest GRun menu.

@clacaputo
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 1, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-60bbc1/23596/summary.html
COMMIT: 03057b3
CMSSW: CMSSW_12_4_X_2022-03-31-2300/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/37286/23596/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3593039
  • DQMHistoTests: Total failures: 8
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3593009
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 200 log files, 45 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@missirol
Copy link
Contributor

missirol commented Apr 2, 2022

urgent

According to #37286 (comment), the plan of MUO-HLT experts is to have this PR (integrated in master and) backported in time for 12_3_0.

("urgent" here means "to be backported in time for 12_3_0")

@cmsbuild cmsbuild added the urgent label Apr 2, 2022
@clacaputo
Copy link
Contributor

+reconstruction

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2022

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

@qliphy
Copy link
Contributor

qliphy commented Apr 5, 2022

+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.

None yet

7 participants