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

Migrate 4D vertexing and downstream reco to full simulation/reconstruction track timestamps and add TOF PID (10_5_X) #25628

Merged
merged 27 commits into from Jan 22, 2019

Conversation

bendavid
Copy link
Contributor

@bendavid bendavid commented Jan 11, 2019

(master version of #25627)
Implements changes needed to propagate full simulation and reconstruction based track timestamps to 4D vertex reconstruction and particle flow.
(Previously this was already implemented and running based on the fastsim timestamps used for the MTD Technical Proposal)

The following strategy is used to deal with particles moving slower than the speed of light:

Existing timestamps from the TrackExtender assume the pion mass hypothesis when propagating the MTD cluster time back to the beamline. The valuemaps produced at this step have been extended to provide some additional information in a more convenient form. (Time at MTD, momentum magnitude of refitted track, etc) As well the uncertainty is inflated by adding in quadrature the time of flight difference between the pion and proton mass hypothesis for each track.

First pass of 4D vertex reconstruction runs on the output as above. After a bugfix/protection, combined with the inflated uncertainties it is able to deal with the resulting outliers from slower kaons/protons in a reasonable way.

Additional TOFPIDProducer which tests compatibility of kaon and proton mass hypotheses for the tracks with the 4D vertices from 2) and produces updated timestamps where appropriate. This producer also creates value maps of particle ID probabilities which can be used downstream.

Second pass of 4D vertex reconstruction based on the refined timestamps from 3)

This pull request also migrates the PFCandidate timestamps to the fullsim/reconstruction ones (previously populated from the fastsim). This automatically propagates to the packed candidates in MINIAOD as well.

An additional collection of slimmed primary vertices with timing is added to MINIAOD, but the PackedCandidate-vertex association, puppi, etc are still based on the default primary vertices without timing.

In order to facilitate comparisons and benchmarking, the fastsim-based timestamps and 4d vertex reco is still run and stored. This is especially relevant since direct comparisons may validate the continued use of existing/fastsim-based results where fullsim-based ones are not yet available.

Some discussion and validation/performance plots in https://indico.cern.ch/event/783100/contributions/3273442/attachments/1780769/2896933/mtdvtxJan17-2019.pdf

@pmeridian @lgray

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@kpedro88
Copy link
Contributor

assign upgrade

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25628/7942

  • This PR adds an extra 44KB to repository

@cmsbuild
Copy link
Contributor

New categories assigned: upgrade

@kpedro88 you have been requested to review this Pull request/Issue and eventually sign? Thanks

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @bendavid (Josh Bendavid) for master.

It involves the following packages:

CommonTools/RecoAlgos
RecoMTD/TrackExtender
RecoParticleFlow/PFProducer
RecoVertex/Configuration
RecoVertex/PrimaryVertexProducer

@perrotta, @cmsbuild, @kpedro88, @slava77 can you please review it and eventually sign? Thanks.
@rappoccio, @jdolen, @makortel, @abbiendi, @GiacomoSguazzoni, @mmarionncern, @jhgoh, @lgray, @ahinzmann, @gkasieczka, @bachtis, @cbernet, @ebrondol, @VinInn, @rovere, @dgulhan, @seemasharmafnal, @clelange this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@slava77
Copy link
Contributor

slava77 commented Jan 11, 2019

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 11, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/32538/console Started: 2019/01/11 19:01

@cmsbuild
Copy link
Contributor

-1

Tested at: f487a29

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25628/32538/summary.html

I found follow errors while testing this PR

Failed tests: Build ClangBuild

  • Build:

I found compilation warning when building: See details on the summary page.

  • Clang:

I found compilation warning while trying to compile with clang. Command used:

USER_CUDA_FLAGS='--expt-relaxed-constexpr' USER_CXXFLAGS='-Wno-register -fsyntax-only' scram build -k -j 16 COMPILER='llvm compile'

See details on the summary page.

@cmsbuild
Copy link
Contributor

Comparison not run due to Build errors (RelVals and Igprof tests were also skipped)

@kpedro88
Copy link
Contributor

+upgrade

@andrius-k
Copy link

+1

@fabiocos
Copy link
Contributor

code-checks

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25628/8127

from Configuration.Eras.Modifier_phase2_timing_cff import phase2_timing
_phase2_timing_slimmingTask = cms.Task(slimmingTask.copy(),
offlineSlimmedPrimaryVertices4D)
phase2_timing.toReplaceWith(slimmingTask,_phase2_timing_slimmingTask)
Copy link
Contributor

Choose a reason for hiding this comment

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

@bendavid @slava77 given the other discussions in the review about phase2_timing vs phase2_timing_layer , is this really the desired behaviour?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this adds an additional slimmed primary vertex collection to the miniaod. In the case of phase2_timing alone, this corresponds to the fastsim 4d vertices, in the case of phase2_timing_layer, this corresponds to fullsim 4d vertices.


_phase2_timing_extraCommands = ["keep *_offlineSlimmedPrimaryVertices4D_*_*"]
from Configuration.Eras.Modifier_phase2_timing_cff import phase2_timing
phase2_timing.toModify(MicroEventContentMC, outputCommands = MicroEventContentMC.outputCommands + _phase2_timing_extraCommands)
Copy link
Contributor

Choose a reason for hiding this comment

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

@bendavid @slava77 given the other discussions in the review about phase2_timing vs phase2_timing_layer , is this really the desired behaviour?

@fabiocos
Copy link
Contributor

+1

@fabiocos
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit 5500bf2 into cms-sw:master Jan 22, 2019
@fabiocos
Copy link
Contributor

@bendavid @pmeridian @slava77 @kpedro88 this PR is creating problems in the scenario D19, the old MTD simulation, as it crashes in the test 20434.0, see

https://cmssdt.cern.ch/SDT/cgi-bin/logreader/slc6_amd64_gcc700/CMSSW_10_5_X_2019-01-22-2300/pyRelValMatrixLogs/run/20434.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D19_GenSimHLBeamSpotFull14+DigiFullTrigger_2023D19+RecoFullGlobal_2023D19+HARVESTFullGlobal_2023D19/step3_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D19_GenSimHLBeamSpotFull14+DigiFullTrigger_2023D19+RecoFullGlobal_2023D19+HARVESTFullGlobal_2023D19.log#/

where in the RECO step3 you get

----- Begin Fatal Exception 23-Jan-2019 05:23:18 CET-----------------------
An exception of category 'ProductNotFound' occurred while
[0] Processing Event run: 1 lumi: 1 event: 5 stream: 1
[1] Running path 'validation_step6'
[2] Prefetching for module CaloParticleValidation/'caloparticlevalidation'
[3] Prefetching for module SimPFProducer/'simPFProducer'
[4] Prefetching for module TOFPIDProducer/'tofPID'
[5] Calling method for module PrimaryVertexProducer/'unsortedOfflinePrimaryVertices4DnoPID'
Exception Message:
Principal::getByToken: Found zero products matching all criteria
Looking for type: edm::ValueMap<float>
Looking for module label: trackExtenderWithMTD
Looking for productInstanceName: generalTracksigmat0

Additional Info:
[a] If you wish to continue processing events after a ProductNotFound exception,
add "SkipEvent = cms.untracked.vstring('ProductNotFound')" to the "options" PSet in the configuration.

----- End Fatal Exception -------------------------------------------------

Do we really want to modify this scenario? My understanding is that @kpedro88 preferred to keep it frozen, waiting for the final new MTD simulation to become official to get rid of it

@fabiocos
Copy link
Contributor

@bendavid what is the purpose of touching the old simulation? Wouldn't be simpler just modify (phase2_timing_layer_tile | phase2_timing_layer_bar) ?

@bendavid
Copy link
Contributor Author

Looking at this now, the old scenario was not supposed to be modified here in the end (except for adding the new collection to miniaod which is generic), but since the configuration files for RecoVertex were heavily restructured, probably something was touched by mistake.

@bendavid
Copy link
Contributor Author

Ok, sorry, we're talking about two different things.
Indeed I was not aware of this, will rework the configs to fix it (but it makes things messy, because there will be phase2_timing_layer_tile | phase2_timing_layer_bar now in several places)

@fabiocos
Copy link
Contributor

fabiocos commented Jan 23, 2019

@bendavid ok, I believe now we understand better, please check https://github.com/fabiocos/cmssw/tree/fc-fixMTD4D
where I have implemented this, both 20434.0 and 27434.0 run smoothly till the end (although I did not check the outputs)

@bendavid
Copy link
Contributor Author

This actually would remove parts of the track and vertex validation which were already present in the old scenario (and using fastsim inputs).

I opened a new PR here which should really preserve the existing behaviour (except for adding the additional slimmed collection in miniaod, which would still be useful/desirable in case this is ever used)
#25745

@kpedro88
Copy link
Contributor

(but it makes things messy, because there will be phase2_timing_layer_tile | phase2_timing_layer_bar now in several places)

I suggested a while ago to make a common phase2_timing_layer_new modifier, and then chain the tile- and bar-specific modifications:

phase2_timing_layer_new = cms.Modifier()
phase2_timing_bar = cms.Modifier()
phase2_timing_tile = cms.Modifier()
phase2_timing_layer_bar = cms.ModifierChain(phase2_timing_layer_new, phase2_timing_bar)
phase2_timing_layer_tile = cms.ModifierChain(phase2_timing_layer_new, phase2_timing_tile)

@makortel
Copy link
Contributor

@kpedro88

(but it makes things messy, because there will be phase2_timing_layer_tile | phase2_timing_layer_bar now in several places)

I suggested a while ago to make a common phase2_timing_layer_new modifier, and then chain the tile- and bar-specific modifications:

phase2_timing_layer_new = cms.Modifier()
phase2_timing_bar = cms.Modifier()
phase2_timing_tile = cms.Modifier()
phase2_timing_layer_bar = cms.ModifierChain(phase2_timing_layer_new, phase2_timing_bar)
phase2_timing_layer_tile = cms.ModifierChain(phase2_timing_layer_new, phase2_timing_tile)

What would the phase2_timing_layer_bar and phase2_timing_layer_tile be used for?

@kpedro88
Copy link
Contributor

To combine with the Phase2 Era to make top-level Eras for workflows. (Not essential, but possibly worthwhile for organization.)

@makortel
Copy link
Contributor

Ok, thanks.

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

9 participants