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

DisplacedGeneralStep in tracking #32036

Merged
merged 2 commits into from Jan 29, 2021
Merged

Conversation

doujadarej
Copy link
Contributor

@doujadarej doujadarej commented Nov 5, 2020

PR description:

Added a tracking iteration after tobTecStep and PixelLess step

PR validation:

Tests have been made on different samples :
Run3 MC :
DisplacedSUSY_stopToBottom_M_800_500mm_TuneCP5_14TeV_pythia8/Run3Summer19DR-106X_mcRun3_2021_realistic_v3-v2/GEN-SIM-DIGI-RAW
/TTbar_14TeV_TuneCP5_Pythia8/Run3Summer19DR-106X_mcRun3_2021_realistic_v3-v2/GEN-SIM-DIGI-RAW 
WminusH_HToSSTobbbb_WToLNu_MH-125_MS-40_ctauS-500_TuneCUETP8M1_14TeV-powheg-pythia8/

Run2 MC :
/TTbar_13TeV_TuneCP5_Pythia8/RunIISummer19UL17DIGI-106X_mc2017_realistic_v6-v1/GEN-SIM-DIGI-RAW

Data :
/MET/Run2017B-HighMET-17Nov2017-v1/RAW-RECO

and several runTheMatrix workflows (basic test procedure suggested in the CMSSW PR instructions and using reconstruction:trackingOnly, trackingOnlyDQM and trackingOnlyValidation for seed monitoring.


commit 'add DisplacedGeneralStep' :
Change name of iteration 
SiStripTripletStep -> DisplacedGeneralStep
Add modifier to switch from default tracking to displacedTracking 
for Phase1 tracking flavour
Remove from ParticleFlow (for now?)
Replace reservedForUpgrades1 (step25) by SiStripTriplet instead
of adding it (Step46)

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 5, 2020

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 5, 2020

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32036/19587

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 5, 2020

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 5, 2020

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32036/19589

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 5, 2020

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

It involves the following packages:

DQM/TrackingMonitorSource
DataFormats/TrackReco
RecoParticleFlow/PFTracking
RecoTracker/FinalTrackSelectors
RecoTracker/IterativeTracking
RecoTracker/TkSeedingLayers
Validation/RecoTrack

@perrotta, @andrius-k, @kmaeshima, @ErnestaP, @cmsbuild, @jfernan2, @fioriNTU, @slava77, @jpata can you please review it and eventually sign? Thanks.
@wmtford, @felicepantaleo, @jandrea, @fioriNTU, @threus, @seemasharmafnal, @mmarionncern, @hdelanno, @makortel, @JanFSchulte, @lgray, @sroychow, @GiacomoSguazzoni, @rovere, @VinInn, @hatakeyamak, @mschrode, @idebruyn, @ebrondol, @mtosi, @dgulhan, @arossi83, @lecriste, @cbernet, @gpetruc this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@jfernan2
Copy link
Contributor

jfernan2 commented Nov 5, 2020

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 5, 2020

The tests are being triggered in jenkins.

@slava77
Copy link
Contributor

slava77 commented Nov 5, 2020

Parameters for seeding, track building and track selection have been tuned in order to lower the fake rate and the extra timing.

IIUC, the new iteration is included by default.

please provide timing comparisons with a recent 11_2_X release as a baseline and this PR for the full reco workflow.
for Run-3 workflow 11834.21, around 100 events will be good.
CPU cost increase of O(1%) to the total reco time will need some significant scrutiny/justification.

a standard set of tracking MTV plots for a Run3 workflow with PU would be useful as well (I guess out of the box matrix it can be 11834.0 or 11834.99).

@@ -138,7 +138,8 @@ namespace reco {
hiRegitMuTobTecStep = 43,
hiRegitMuMuonSeededStepInOut = 44,
hiRegitMuMuonSeededStepOutIn = 45,
algoSize = 46
siStripTripletStep = 46,
algoSize = 47
Copy link
Contributor

Choose a reason for hiding this comment

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

How about using the value of reservedForUpgrades1 instead? (at the time I meant it exactly for this kind of situation)

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense; I guess the idea is to rename it.
BTW, isn't the siStripTriplet confusing? pixel-less iteration is also a strip triplet iteration
also, what if down the road we decide to make it use 4 hits?

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense; I guess the idea is to rename it.

Exactly, do

       detachedQuadStep = 24,
-      reservedForUpgrades1 = 25,
+      siStripTiripletStep = 25,
       reservedForUpgrades2 = 26,

That would avoid the class version changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

it might be that a new dedicated step (again targeting displaced tracking) would be added for Run3, in that case how should we proceed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I see there is room for another one (reservedForUpgrades2)
Replacing reservedForUpgrades1 by SiStripTripletStep indeed makes it much simpler without changing much the structure (already tried)

Copy link
Contributor

Choose a reason for hiding this comment

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

if another "regional" displaced iteration is planned, how about we plan to use the reservedForUpgrades1 and reservedForUpgrades2
and name the proposed iteration displacedGeneralStep while the one from Andrew will be displacedRegionalStep?

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks, sounds good to me, indeed

@doujadarej
Copy link
Contributor Author

@slava77 @makortel thanks for your comments. What if we add a nonDefaultEraName like trackingLowPU, trackingPhase1... e.g "displacedTracking" in which we would enable the iteration in the sequence? That was actually the first intention
@jandrea @mtosi

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 5, 2020

-1

Tested at: cf75f2b

CMSSW: CMSSW_11_2_X_2020-11-05-1100
SCRAM_ARCH: slc7_amd64_gcc820
You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f8f7ff/10531/summary.html

I found follow errors while testing this PR

Failed tests: Build

  • Build:

I found compilation error when building:

>> Building LCG reflex dict from header file src/DataFormats/GsfTrackReco/src/classes.h
>> Compiling LCG dictionary: tmp/slc7_amd64_gcc820/src/DataFormats/GsfTrackReco/src/DataFormatsGsfTrackReco/a/DataFormatsGsfTrackReco_xr.cc
>> Building  shared library tmp/slc7_amd64_gcc820/src/DataFormats/GsfTrackReco/src/DataFormatsGsfTrackReco/libDataFormatsGsfTrackReco.so
Copying tmp/slc7_amd64_gcc820/src/DataFormats/GsfTrackReco/src/DataFormatsGsfTrackReco/libDataFormatsGsfTrackReco.so to productstore area:
>> Checking EDM Class Version for src/DataFormats/GsfTrackReco/src/classes_def.xml in libDataFormatsGsfTrackReco.so
error: class 'reco::GsfTrack' has a different checksum for ClassVersion 20. Increment ClassVersion to 21 and assign it to checksum 866972163
Suggestion: You can run 'scram build updateclassversion' to generate src/DataFormats/GsfTrackReco/src/classes_def.xml.generated with updated ClassVersion
gmake: *** [tmp/slc7_amd64_gcc820/src/DataFormats/GsfTrackReco/src/DataFormatsGsfTrackReco/libDataFormatsGsfTrackReco.so] Error 1
Leaving library rule at DataFormats/GsfTrackReco
>> Leaving Package DataFormats/GsfTrackReco
>> Package DataFormats/GsfTrackReco built


@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 5, 2020

Comparison job queued.

@doujadarej
Copy link
Contributor Author

@slava77
In last commit

  • changed number of maxElement for doublets to the same as PixelLess and TobTec (50000000)
  • Impact of onlyPixelHitsForSeedCleaner : No big difference from the tests I ran, but no loss of good tracks when I turn it off. It will probably help with fakes, but need to run on more stats. Thanks for pointing this out. Used false, like in PixelLess
  • Applied SiStripClusterChargeCutTight for all layers after tests
  • Changed the cluster names to the right one
  • put the line breaks back

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32036/20881

@cmsbuild
Copy link
Contributor

Pull request #32036 was updated. @perrotta, @andrius-k, @kmaeshima, @ErnestaP, @cmsbuild, @silviodonato, @franzoni, @jfernan2, @fioriNTU, @slava77, @jpata, @qliphy, @fabiocos, @davidlange6 can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Jan 25, 2021

@cmsbuild please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f8f7ff/12517/summary.html
COMMIT: 6d6a987
CMSSW: CMSSW_11_3_X_2021-01-24-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/32036/12517/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: 37
  • DQMHistoTests: Total histograms compared: 2716961
  • DQMHistoTests: Total failures: 7
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2716932
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 1.794 KiB( 36 files compared)
  • DQMHistoSizes: changed ( 1000.0,... ): 0.078 KiB Tracking/MessageLog
  • Checked 156 log files, 37 edm output root files, 37 DQM output files

@jfernan2
Copy link
Contributor

@doujadarej this PR only modifies Tracking/MessageLog folders in DQM, I understand that plots are being added in
#32157

From your PR description I would have expected some DQM modifications. Can you confirm?

@doujadarej
Copy link
Contributor Author

@doujadarej this PR only modifies Tracking/MessageLog folders in DQM, I understand that plots are being added in
#32157

From your PR description I would have expected some DQM modifications. Can you confirm?

@jfernan2 , I'm not sure what you mean, sorry.
The extra iteration appears in the DQM plots when it's enabled, in the track collections and also in the seeding and track building monitoring. But it's independent from #32157 which is for all collections. Of course when both of them are enabled, they work together (already tested).

@jfernan2
Copy link
Contributor

Hi @doujadarej
Now it is clear. Thanks

@jfernan2
Copy link
Contributor

+1

@slava77
Copy link
Contributor

slava77 commented Jan 26, 2021

+1

for #32036 6d6a987

  • code changes are in line with the PR description and the follow up review
    • the default tracking setup is not changed
    • the new iteration is available via --procModifiers displacedTracking; following the discussion(s) in the RECO meeting of Nov 17 https://indico.cern.ch/event/978240/ (and other means) this is still a work in progress and may need improvements for a more general use, although special rerecos with a specific focus on displaced track signals may already use it as it
  • jenkins tests pass and comparisons with the baseline show no (relevant) differences as expected
  • a local test based on 2021 workflow using ttbar with PU (11834) with a modification to use displacedTracking shows roughly expected result
    • reco time about doubles, dominated by displacedGeneralStepTrackCandidates and displacedGeneralStepHitTriplets
    • there is some increase in efficiency for tracks with higher vtxpos, although the fakerate increase is more substantial (the ttbar sample doesn't really have much real displaced signal tracks though).

wf11834disp_gen09_eff_vpos

wf11834disp_gen09_fr_vpos

@silviodonato
Copy link
Contributor

+1

@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 be automatically merged.

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

10 participants