search

cms-sw / cmssw

CMS Offline Software
http://cms-sw.github.io/
Apache License 2.0
1.07k stars 4.29k forks source link

Proposed removal of TrackTimeValueMapProducer #28479

Open kpedro88 opened 4 years ago

kpedro88 commented 4 years ago

The TrackTimeValueMapProducer was used for the MTD "fastsim" approach (applying simple smearing to the truth time values, no relationship to the actual fast simulation of the CMS detector). Since this approach is no longer needed, I propose removing the unnecessary components.

The branch implementing this proposal is here: https://github.com/cms-sw/cmssw/compare/master...kpedro88:Phase2-WF52

attn: @bendavid @lgray @fabiocos @pmeridian @parbol

kpedro88 commented 4 years ago

assign upgrade

cmsbuild commented 4 years ago

New categories assigned: upgrade

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

cmsbuild commented 4 years ago

A new Issue was created by @kpedro88 Kevin Pedro.

@davidlange6, @Dr15Jones, @smuzaffar, @fabiocos, @kpedro88 can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

smuzaffar commented 3 years ago

@cms-sw/upgrade-l2 , any objections on removing TrackTimeValueMapProducer ? @kpedro88 may be you can open a PR to remove it?

kpedro88 commented 3 years ago

@cms-sw/mtd-dpg-l2 need to indicate what, if any, pieces of this functionality are still needed in modern releases.

kpedro88 commented 3 years ago

also @smuzaffar why did the bot remove the upgrade-pending label?

smuzaffar commented 3 years ago

ah looks like a bug/feature of bot :-) as you are no longer L2 of any category so bot ignored your "assign upgrade" comment

srimanob commented 3 years ago

assign upgrade

cmsbuild commented 3 years ago

New categories assigned: upgrade

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

srimanob commented 3 years ago

assign mtd-dpg

cmsbuild commented 3 years ago

New categories assigned: mtd-dpg

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

fabiocos commented 3 years ago

I am not aware of recent (or even not so recent) uses of the simulation-based time smearing in MTD performance studies. Nowadays we have a reasonable simulation of MTD which allows us to reconstruct timing from the expected behaviour of the detector, without relying on the MC truth to assign time to the vertices.

As far as I can see, removing TrackTimeValueMapProducer implies cleaning RecoVertex/Configuration and RecoParticleFlow/PFProducer, and in practice causes the modifier phase2_timing to become obsolete. Of course the EventContent should be cleaned correspondigly, since we still have:

11:12 farmui01 661> edmDumpEventContent step3.root | grep TimeValueMap
edm::ValueMap<float>                  "trackTimeValueMapProducer"   "generalTracksConfigurableFlatResolutionModel"   "RECO"    
edm::ValueMap<float>                  "trackTimeValueMapProducer"   "generalTracksConfigurableFlatResolutionModelResolution"   "RECO"    
edm::ValueMap<float>                  "trackTimeValueMapProducer"   "generalTracksPerfectResolutionModel"   "RECO"    
edm::ValueMap<float>                  "trackTimeValueMapProducer"   "generalTracksPerfectResolutionModelResolution"   "RECO"

@bendavid please clarify if you still see any interest in keeping this code in the release, which will anyway stay in older releases. Otherwise I may provide a PR where the use of this code is removed.

fabiocos commented 3 years ago

phase2_timing is also used independently by ECAL as far as I can see, so even if the trackTimeValueMapProducer is removed it might not become completely obsolete

srimanob commented 3 years ago

@cms-sw/ecal-dpg-l2 Do I understand correctly that ECAL uses phase2_timing but independently from MTD part, i.e. uses for ECAL timing but no need of MTD information?

@kdlong @juska Could you please comment on above question by @fabiocos

Thanks.

juska commented 3 years ago

I have no idea, maybe Kenneth does but he's on vacation this week. Should we ask somebody from ECAL maybe?

On 9/28/21 4:06 PM, Phat Srimanobhas wrote:

@cms-sw/ecal-dpg-l2 https://github.com/orgs/cms-sw/teams/ecal-dpg-l2 Do I understand correctly that ECAL uses |phase2_timing| but independently from MTD part, i.e. uses for ECAL timing but no need of MTD information?

@kdlong https://github.com/kdlong @juska https://github.com/juska Could you please comment on above question by @fabiocos https://github.com/fabiocos

Thanks.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cms-sw/cmssw/issues/28479#issuecomment-929263655, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABZJZIEW45BFNS5Y54UQ5LTUEHDVBANCNFSM4JR2DWGQ. Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

srimanob commented 3 years ago

Hi @juska Oh, I mean the question on keeping trackTimeValueMapProducer https://github.com/cms-sw/cmssw/issues/28479#issuecomment-929008175

Sorry for my quick text, and make confusion.

juska commented 3 years ago

Ok thanks for the clarification @srimanob . Unfortunately I still don't have an educated opinion. Maybe we should wait for @bendavid to respond.

bendavid commented 3 years ago

I could imagine that the "fast sim" approach might still be useful for debugging purposes (ie comparing "realistic" vs "ideal" timing information for input to 4d vertexing or other downstream algorithms), though I'm not aware of any use along these lines recently.

fabiocos commented 3 years ago

@bendavid we have asked our UPSG contact to investigate whether there is any practical use of this nowadays, although I am not so sure about what mat be really learnt nowadays from a comparison with a MC-truth smearing based reconstruction. I have a branch ready for the suppression of this class https://github.com/fabiocos/cmssw/commits/fc-timevaluemap but before proposing it I will wait for feedback.

thomreis commented 3 years ago

The ECAL phase2_timing use is independent of the MTD as far as I understand. This was used for early studies of a timing layer by slicing ECAL and taking a time out of one of these slides. The corresponding producer is now obsolete and we will probably remove it as well.

fabiocos commented 3 years ago

If I interpret correctly the comment of @thomreis about the ECAL contribution to phase2_timing, that modifier could become completely obsolete

thomreis commented 3 years ago

At least we (ECAL) will not use it anymore, yes.

srimanob commented 1 year ago

@fabiocos @thomreis Back to 1.5 year discussion, is tis cleaning happen already? Thx.

fabiocos commented 1 year ago

@srimanob not really that I know. I had a branch ready for this, but after the request to make urgently studies of performances about one year ago where this might have been used (although it was not) I have kept this on hold