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

Implemented new strategy to generate OI seeds for displaced muons #38076

Merged
merged 4 commits into from May 31, 2022

Conversation

alexsr-98
Copy link
Contributor

PR description:

This PR implements a new iteration to generate OI seeds for displaced muons. To activate this iteration a flag is added in order to be able to turn off this part of the code and don't interfere with the current algorithm. These slides contain information regarding the problem that we observed and this solution. In slide 7, the performance is shown (compare yellow and brown histograms) and more information regarding the rates and timing is in the backup.

To briefly summarise the PR we have:

Added parameters:

  • displacedReco: flag to turn on this seeding.
  • SFHld and SFHd: scale factors to fine tune the displaced seeding.

Algorithm:

  • The idea is to iterate over all the TOB and TEC layers but removing the L2 eta cut that can kill some potential seeds for displaced candidates (see slides).
  • We generate both hitless and hit seeds but using as input the TSOS at the outer tracker boundary instead of the one updated at IP (tsosAtIP).

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38076/30164

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @alexsr-98 for master.

It involves the following packages:

  • RecoMuon/TrackerSeedGenerator (reconstruction)

@jpata, @cmsbuild, @clacaputo, @slava77 can you please review it and eventually sign? Thanks.
@HuguesBrun, @abbiendi, @Fedespring, @bellan, @sscruz, @jhgoh, @CeliaFernandez, @trocino, @cericeci, @rociovilar 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

@jpata
Copy link
Contributor

jpata commented May 25, 2022

@cmsbuild please test

@jpata
Copy link
Contributor

jpata commented May 25, 2022

type muon

@clacaputo
Copy link
Contributor

assign hlt

@cmsbuild
Copy link
Contributor

New categories assigned: hlt

@missirol,@Martin-Grunewald you have been requested to review this Pull request/Issue and eventually sign? Thanks

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-df21e5/24988/summary.html
COMMIT: 733064c
CMSSW: CMSSW_12_5_X_2022-05-24-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38076/24988/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: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3650985
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3650961
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 208 log files, 45 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

if (displacedReco_ && outerTkStateOutside.isValid()) {
layerCount = 0;
for (auto it = tob.rbegin(); it != tob.rend(); ++it) {
LogTrace("TSGForOIFromL2") << "TSGForOIFromL2::produce: looping in TOB layer " << layerCount << std::endl;
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
LogTrace("TSGForOIFromL2") << "TSGForOIFromL2::produce: looping in TOB layer " << layerCount << std::endl;
LogTrace("TSGForOIFromL2") << "TSGForOIFromL2::produce: looping in TOB layer " << layerCount;

out);
}
LogTrace("TSGForOIFromL2") << "TSGForOIFromL2:::produce: NumSeedsMade = " << numSeedsMade
<< " , layerCount = " << layerCount << std::endl;
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
<< " , layerCount = " << layerCount << std::endl;
<< " , layerCount = " << layerCount;

<< " , layerCount = " << layerCount << std::endl;
}

if (L2muonEta < 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.

Suggested change
if (L2muonEta < 0.0) {
else if (L2muonEta < 0.0) {

What should happen in the case L2muonEta==0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can enter the first, but I think It doesn't really matter.

out);
}
LogTrace("TSGForOIFromL2") << "TSGForOIFromL2:::produce: NumSeedsMade = " << numSeedsMade
<< " , layerCount = " << layerCount << std::endl;
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
<< " , layerCount = " << layerCount << std::endl;
<< " , layerCount = " << layerCount;

if (L2muonEta < 0.0) {
layerCount = 0;
for (auto it = tecNegative.rbegin(); it != tecNegative.rend(); ++it) {
LogTrace("TSGForOIFromL2") << "TSGForOIFromL2::produce: looping in TEC- layer " << layerCount << std::endl;
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
LogTrace("TSGForOIFromL2") << "TSGForOIFromL2::produce: looping in TEC- layer " << layerCount << std::endl;
LogTrace("TSGForOIFromL2") << "TSGForOIFromL2::produce: looping in TEC- layer " << layerCount;

out);
}
LogTrace("TSGForOIFromL2") << "TSGForOIFromL2:::produce: NumSeedsMade = " << numSeedsMade
<< " , layerCount = " << layerCount << std::endl;
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
<< " , layerCount = " << layerCount << std::endl;
<< " , layerCount = " << layerCount;

@@ -425,7 +517,7 @@ void TSGForOIFromL2::makeSeedsFromHits(const GeometricSearchDet& layer,
hitSeedsMade++;
if (found == numOfHitsToTry_)
break;
if (hitSeedsMade > maxHitSeeds_)
if (hitSeedsMade > maxHitSeeds_) //
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
if (hitSeedsMade > maxHitSeeds_) //
if (hitSeedsMade > maxHitSeeds_)

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38076/30192

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38076/30197

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

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

@missirol
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-df21e5/25019/summary.html
COMMIT: 37b9747
CMSSW: CMSSW_12_5_X_2022-05-26-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38076/25019/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: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3652266
  • DQMHistoTests: Total failures: 8
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3652236
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 208 log files, 45 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@missirol
Copy link
Contributor

+hlt

@alexsr-98 , does this PR need to be backported to 12_4_X to be used at HLT during 2022 pp data-taking?

@alexsr-98
Copy link
Contributor Author

Hi @missirol, yes we need this, do we have to do something to backport it?

@missirol
Copy link
Contributor

I think the backport of this PR should be uncontroversial, as this PR only introduces a new option disabled by default.

@cms-sw/reconstruction-l2 , please consider reviewing this PR soonish, since there is a 12_4_X backport involved.

do we have to do something to backport it?

Yes. Once this PR is merged, you need to open a backport of (the final version of) this PR to the branch CMSSW_12_4_X.

Note: the deadline for the next CMSSW_12_4_X release is Jun-15, so the backport should be done well ahead of that date.

What is the ETA for the changes to the HLT menu (via JIRA ticket) which would make use of this PR?

@alexsr-98
Copy link
Contributor Author

@missirol Okay I will open the backport as soon as I can.

Regarding the last question, we have open a JIRA ticket for this (https://its.cern.ch/jira/browse/CMSHLT-2326) to try to implement this as soon as possible.

@clacaputo
Copy link
Contributor

+reconstruction

  • new strategy for displacedMuons OI seeds generation
    • added new option displacedReco set to false by default
  • no reco changes observed

@cmsbuild
Copy link
Contributor

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 May 31, 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

6 participants