update deepCore for Run3 #33531

mtosi · 2021-04-26T19:53:23Z

PR description:

during the special deepCore validation [1]
it has been highlighted that the current implementation of the jetCore for Run3 is suboptimal

as presented at the RECO meeting, the first approach is to

apply the old seeding of the jetCore step in the "endcap" region
apply the new deepCore seeding to jets w/ pT > 100 GeV (instead of the currently 300 GeV)
build seeds via deepCore in a cone of at least 0.25 wrt the jet axis (instead of the current 0.1)

this PR adjusts these points

[1]
https://its.cern.ch/jira/browse/PDMVRELVALS-107

PR validation:

the code has been explicitly tested on QCD high pT bin sample w/o PU
applying the --procModifiers seedingDeepCore

there is a not negligible increase in the seeds multiplicity, but its purity is better
efficiency

fakerate

seeding fake vs collection

seeding efficiency vs collection

FOR TESTING

TTbar : runTheMatrix --what upgrade -l 11634.17
QCD high pt : runTheMatrix --what upgrade -l 11723.17

cmsbuild · 2021-04-26T19:59:02Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33531/22301

This PR adds an extra 24KB 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-33531/22301/code-format.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33531/22301/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2021-04-26T20:09:50Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33531/22302

This PR adds an extra 24KB to repository

cmsbuild · 2021-04-26T20:10:12Z

A new Pull Request was created by @mtosi (mia tosi) for master.

It involves the following packages:

RecoTracker/IterativeTracking
RecoTracker/TkSeedGenerator

@perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@makortel, @felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @gpetruc, @ebrondol, @mtosi, @dgulhan 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

makortel · 2021-04-26T21:36:52Z

RecoTracker/IterativeTracking/python/JetCoreRegionalStep_cff.py

+                                         jetCoreRegionalStepEndcapTracks,
+                                         #                                   jetCoreRegionalStepClassifier1,jetCoreRegionalStepClassifier2,
+                                         jetCoreRegionalStepEndcap)
+JetCoreRegionalEndcapStep = cms.Sequence(JetCoreRegionalStepEndcapTask)


Do you really want to (potentially) filter events?

uhmmmm, I'm sorry but I'm not super sure I get this question

the structure was already like that, I simply double the Tasks in order to handle independently the barrel and the endcap regions

sorry for this, but could you please comment further ?

opsi

jetsForCoreTrackingBarrel = cms.EDFilter('CandPtrSelector', src = cms.InputTag('ak4CaloJetsForTrk'), cut = cms.string('pt > 100 && abs(eta) < 2.5')) jetsForCoreTrackingEndcap = cms.EDFilter('CandPtrSelector', src = cms.InputTag('ak4CaloJetsForTrk'), cut = cms.string('pt > 100 && abs(eta) > 1.4 && abs(eta) < 2.5'))

Very sorry, nevermind, I read the configuration too quickly and somehow thought the jetsForCoreTrackingBarrel and jetsForCoreTrackingEndcap were placed directly in these Sequences. When they're placed in Tasks (as they are), framework implicitly ignores the event filtering decision.

Are the intermediate Sequences JetCoreRegionalBarrelStep, JetCoreRegionalEndcapStep, and allJetCoreRegionalStep needed outside of this file? If not, the replacement logic could be simplified to something along

seedingDeepCore.toReplaceWith(JetCoreRegionalStepTask, cms.Task( JetCoreRegionalStepBarrelTask, JetCoreRegionalStepEndcapTask ))

thanks @makortel, I'm going to implement your suggestion

mtosi · 2021-04-27T12:43:00Z

I've just added the filter parameter explicitly and set it to False

cmsbuild · 2021-04-27T12:49:21Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33531/22315

This PR adds an extra 24KB to repository

cmsbuild · 2021-04-27T12:49:41Z

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

makortel · 2021-04-27T13:22:28Z

I don't see anything consuming jetCoreRegionalStepBarrel and jetCoreRegionalStepEndcap, is that intentional? (if nothing consumes them, they won't get run, and all producers consumed only by them won't get run etc)

mtosi · 2021-04-27T19:57:28Z

I have just tested the following, and it seems to work (even if I still have to handle both the seed validation and DQM of this step in the proper way)

I'm not happy about it, though
as you see I had to double the same come in order to have both the TrackCollection and the corresponding ValueMap :(
I'm not merging the seed collections, because the track building may be different (in particular we are expecting Valerio to adjust the rebuilding in order to exploit the better track parameters estimation provided by the DeepCore seed ...)

do you have any suggestions ?
I would really happy in implementing it

thanks

from RecoTracker.FinalTrackSelectors.TrackCollectionMerger_cfi import *
_deepCore_jetCoreRegionalStepTracks = TrackCollectionMerger.clone(
    trackProducers   = ["jetCoreRegionalStepBarrelTracks",
			"jetCoreRegionalStepEndcapTracks",],
    inputClassifiers = ["jetCoreRegionalStepBarrel",
			"jetCoreRegionalStepEndcap",],
    foundHitBonus    = 100.0,
    lostHitPenalty   = 1.0
)

seedingDeepCore.toReplaceWith(jetCoreRegionalStepTracks,_deepCore_jetCoreRegionalStepTracks)

_jetCoreRegionalStep = jetCoreRegionalStepTracks.clone()
seedingDeepCore.toReplaceWith(jetCoreRegionalStep,_jetCoreRegionalStep)

seedingDeepCore.toReplaceWith(JetCoreRegionalStepTask, cms.Task(
    JetCoreRegionalStepBarrelTask,
    JetCoreRegionalStepEndcapTask,
    cms.Task(jetCoreRegionalStepTracks,jetCoreRegionalStep)
)
)

makortel · 2021-04-28T00:05:20Z

I believe it's either that or dealing with two jetCore track collections in the earlyGeneralTracks etc (and I've forgotten the difference between TrackCollectionMerger and TrackListMerger). Your suggestion can be shortened a little bit

from RecoTracker.FinalTrackSelectors.TrackCollectionMerger_cfi import *
seedingDeepCore.toReplaceWith(jetCoreRegionalStepTracks, TrackCollectionMerger.clone(
    trackProducers   = ["jetCoreRegionalStepBarrelTracks",
			"jetCoreRegionalStepEndcapTracks",],
    inputClassifiers = ["jetCoreRegionalStepBarrel",
			"jetCoreRegionalStepEndcap",],
    foundHitBonus    = 100.0,
    lostHitPenalty   = 1.0
))

seedingDeepCore.toReplaceWith(jetCoreRegionalStep, jetCoreRegionalStepTracks.clone()) #(*)

seedingDeepCore.toReplaceWith(JetCoreRegionalStepTask, cms.Task(
    JetCoreRegionalStepBarrelTask,
    JetCoreRegionalStepEndcapTask,
    jetCoreRegionalStepTracks,
    jetCoreRegionalStep
))

The duplication of the TrackListMerger could be avoided with EDAlias. That would be along

# rename jetCoreRegionalStep to e.g. jetCoreRegionalStepImpl

# alias the MVAValues and QualityMasks products from jetCoreRegionalStepImpl to jetCoreRegionalStep
jetCoreRegionalStep = cms.EDAlias(
    jetCoreRegionalStepImpl = cms.VPSet(
        cms.PSet(type = cms.string("floats")),
        cms.PSet(type = cms.string("uchars")),
    )
)

from RecoTracker.FinalTrackSelectors.TrackCollectionMerger_cfi import *
seedingDeepCore.toReplaceWith(jetCoreRegionalStepTracks, TrackCollectionMerger.clone(
    trackProducers   = ["jetCoreRegionalStepBarrelTracks",
			"jetCoreRegionalStepEndcapTracks",],
    inputClassifiers = ["jetCoreRegionalStepBarrel",
			"jetCoreRegionalStepEndcap",],
    foundHitBonus    = 100.0,
    lostHitPenalty   = 1.0
))

# change the alias-from to point to jetCoreRegionalStepTracks
seedingDeepCore.toModify(jetCoreRegionalStep,
    jetCoreRegionalStepImpl = None,
    jetCoreRegionalStepTracks = jetCoreRegionalStep.jetCoreRegionalStepImpl.copy()
)

seedingDeepCore.toReplaceWith(JetCoreRegionalStepTask, cms.Task(
    JetCoreRegionalStepBarrelTask,
    JetCoreRegionalStepEndcapTask,
    jetCoreRegionalStepTracks,
    jetCoreRegionalStep
))

mtosi · 2021-04-28T18:01:41Z

thanks @makortel I'm following your suggestion ;)

about the seed validation, do you think it would make sense to merge the seed collections just before the corresponding MTV (and DQM) call ? probably not, just add the 2 separate collections, right ?

mtosi · 2021-04-28T18:03:35Z

uhmm, I got the following error
Exception Message: unknown python problem occurred. RuntimeError: Adding an entry of type 'EDAlias'to a Task. It is illegal to add this type to a Task.

makortel · 2021-04-28T18:08:56Z

uhmm, I got the following error

EDAlias does not need to (and should not be) added to any Task or Sequence. It is enough to just define it, and as long as it reaches the cms.Process the aliasing will be establised.

cmsbuild · 2021-05-18T16:05:04Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33531/22726

This PR adds an extra 48KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File DQM/TrackingMonitorSource/python/IterTrackingModules4seedMonitoring_cfi.py modified in PR(s): EXO regional tracking iteration #32806
- File RecoTracker/IterativeTracking/python/iterativeTkConfig.py modified in PR(s): EXO regional tracking iteration #32806
- File Validation/RecoTrack/python/TrackValidation_cff.py modified in PR(s): EXO regional tracking iteration #32806

cmsbuild · 2021-05-18T16:05:29Z

Pull request #33531 was updated. @perrotta, @andrius-k, @kmaeshima, @ErnestaP, @ahmad3213, @cmsbuild, @jfernan2, @slava77, @jpata, @rvenditti can you please check and sign again.

slava77 · 2021-05-20T01:17:00Z

@cmsbuild please test

cmsbuild · 2021-05-20T05:23:44Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-dd4e20/15197/summary.html
COMMIT: 0fbf16e
CMSSW: CMSSW_12_0_X_2021-05-19-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/33531/15197/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS Clang-Tidy warnings: There are Clang-Tidy warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-dd4e20/15197/llvm-analysis/cmsclangtidy.txt for details.

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-dd4e20/11723.17_QCD_Pt_1800_2400_14+2021_seedingDeepCore+QCD_Pt_1800_2400_14TeV_TuneCP5_GenSim+Digi+Reco+HARVEST

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2648242
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2648219
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
Checked 155 log files, 37 edm output root files, 37 DQM output files
TriggerResults: no differences found

jfernan2 · 2021-05-20T08:07:40Z

+1

slava77 · 2021-05-22T00:41:53Z

while making a comparison of this PR with seedingDeepCore enabled with the baseline using the default jet core, it looks like I rediscovered for myself the large loss of efficiency for displaced tracks in the deepCore.
Looking back at the older deepCore-only relval comparisons, we can see the same
(/RelValQCD_Pt_600_800_14/CMSSW_11_3_0_pre3-113X_mcRun3_2021_realistic_v4 orig is in blue)
https://tinyurl.com/yhsqh569
jetCore alone:

it looks like there is a very strong preference to longer tracks

Is it something to be looked at before the deepCore can go to production?

slava77 · 2021-05-22T01:49:35Z

+reconstruction

for #33531 0fbf16e

code changes are in line with the PR description and the follow up review
- previously posted summary of pending items in update deepCore for Run3 #33531 (comment) is essentially complete
jenkins tests pass and comparisons with the baseline show no differences
local tests with seedingDeepCore enabled confirm that the new modules are running with more tracks in a larger dR, smaller (jet) region pt, and now with more tracks in higher eta

cmsbuild · 2021-05-22T01:49:59Z

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

vberta · 2021-05-22T10:15:05Z

while making a comparison of this PR with seedingDeepCore enabled with the baseline using the default jet core, it looks like I rediscovered for myself the large loss of efficiency for displaced tracks in the deepCore.
Looking back at the older deepCore-only relval comparisons, we can see the same
(/RelValQCD_Pt_600_800_14/CMSSW_11_3_0_pre3-113X_mcRun3_2021_realistic_v4 orig is in blue)
https://tinyurl.com/yhsqh569
jetCore alone:

it looks like there is a very strong preference to longer tracks

Is it something to be looked at before the deepCore can go to production?

Hi Slava, thank you for the testing. About the second plot, this does not surprise me: DeepCore produces the seed using the four pixel detector layer info, producing the seed on L2. If a layer is missing (acceptance, broken module...) the performance decreases, but there are some ad-hoc cuts tuning to face this condition in the seed building. if 2 layers are missing the performance of the production start to be very bad, because it is predcting the helix parameters with 2 points only in the messy environment of the jet cores. So yes, definitely DeepCore has a preference for tracks with all the layer of pixel detector.
It can be a problem in very displaced-tracks cases? yes, and if we really need DeepCore in these cases, I can suggest a dedicated training of the NN.
In addition, currently we know that the rebuilding of the seeding region is done in a not-optimal way (old jetcore approach also for DeepCore-seeded tracks), thus the pixel layer counting maybe is not the best figure of merit. I have to say that in these days I'm working exactly on that, to implement the proper approach.
I attach a picture from a march2020 presentation about that (done with |eta|<1.4 cut, on QCD1800-2400 sample)

qliphy · 2021-05-24T00:22:29Z

+1

cmsbuild added this to the CMSSW_12_0_X milestone Apr 26, 2021

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Apr 26, 2021

cmsbuild added code-checks-rejected code-checks-pending and removed code-checks-pending code-checks-rejected labels Apr 26, 2021

mtosi marked this pull request as ready for review April 26, 2021 20:06

cmsbuild added code-checks-approved and removed code-checks-pending labels Apr 26, 2021

makortel reviewed Apr 26, 2021

View reviewed changes

cmsbuild added code-checks-pending and removed code-checks-approved labels Apr 27, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Apr 27, 2021

cmsbuild added code-checks-pending and removed code-checks-approved labels May 18, 2021

fix

0fbf16e

cmsbuild added code-checks-approved and removed code-checks-pending labels May 18, 2021

cmsbuild added tests-started and removed tests-pending labels May 20, 2021

cmsbuild added tests-approved and removed tests-started labels May 20, 2021

cmsbuild added dqm-approved and removed dqm-pending labels May 20, 2021

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels May 22, 2021

cmsbuild added orp-approved and removed orp-pending labels May 24, 2021

cmsbuild merged commit 623cc6d into cms-sw:master May 24, 2021

cmsbuild mentioned this pull request May 24, 2021

Fix import in Phase1L1TJetSumsProducer_cfi for python 3 #33822

Merged

slava77 mentioned this pull request Jun 3, 2021

Migrate module configurations in RecoTracker{IterativeTracking} to use default cfipython #33903

Closed

update deepCore for Run3 #33531

update deepCore for Run3 #33531

Uh oh!

Conversation

mtosi commented Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR description:

PR validation:

Uh oh!

cmsbuild commented Apr 26, 2021

Uh oh!

cmsbuild commented Apr 26, 2021

Uh oh!

cmsbuild commented Apr 26, 2021

Uh oh!

makortel Apr 26, 2021

Choose a reason for hiding this comment

Uh oh!

mtosi Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

mtosi Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

makortel Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

mtosi Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

mtosi commented Apr 27, 2021

Uh oh!

cmsbuild commented Apr 27, 2021

Uh oh!

cmsbuild commented Apr 27, 2021

Uh oh!

makortel commented Apr 27, 2021

Uh oh!

mtosi commented Apr 27, 2021

Uh oh!

makortel commented Apr 28, 2021

Uh oh!

mtosi commented Apr 28, 2021

Uh oh!

mtosi commented Apr 28, 2021

Uh oh!

makortel commented Apr 28, 2021

Uh oh!

cmsbuild commented May 18, 2021

Uh oh!

cmsbuild commented May 18, 2021

Uh oh!

slava77 commented May 20, 2021

Uh oh!

cmsbuild commented May 20, 2021

Comparison Summary

Uh oh!

jfernan2 commented May 20, 2021

Uh oh!

slava77 commented May 22, 2021

Uh oh!

slava77 commented May 22, 2021

Uh oh!

cmsbuild commented May 22, 2021

Uh oh!

vberta commented May 22, 2021

Uh oh!

qliphy commented May 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

mtosi commented Apr 26, 2021 •

edited

Loading