MTD reconstruction: combine the time fro two ETL discs in TrackExtenderWithMTD

[fabiocos]

PR description:

The D76 geometry scenario has two discs for each ETL side, and they are used by the TrackExtenderWithMTD while adding hits to the track for refit, but so far only the time of the first hit was used as timing information, because the code was originally developed for the one-hit disc scenario. This PR combines the time fro the hits associated to a track in two discs by extrapolating the outermost to the innermost, and computing a weighted average. The uncertainty in the subtracted time-of-flight between the two discs induced by the mass hypothesis (pion is used for the extrapolation) is added in the form of the difference between the pion and proton estimated tof. This has very small effect but for very soft tracks.

PR validation:

Code compiles and runs, debugging printouts show the desired behaviour on wf 34634.0.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33226/21670

• This PR adds an extra 20KB to repository

[cmsbuild]

A new Pull Request was created by @fabiocos (Fabio Cossutti) for master.

It involves the following packages:

RecoMTD/TrackExtender

@perrotta, @kpedro88, @cmsbuild, @srimanob, @slava77, @jpata can you please review it and eventually sign? Thanks.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

[fabiocos]

please test

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-bf199f/13632/summary.html
COMMIT: 05030c4
CMSSW: CMSSW_11_3_X_2021-03-19-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/33226/13632/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: 140 differences found in the comparisons
• DQMHistoTests: Total files compared: 37
• DQMHistoTests: Total histograms compared: 2639935
• DQMHistoTests: Total failures: 762
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 2639151
• 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

[perrotta]

Some additional comment: I'm done with the code inspection now

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33226/21718

• This PR adds an extra 20KB to repository

[fabiocos]

@perrotta thanks for cross checking. With @martatornago we are testing an alternative algorithm to search for compatibleDets in ETL layers that could improve that part, which is called in the extender for forward tracks and deteriorated in terms of CPU performances when moving from a ring-based geometry to a sector-based one. Anyway this is still in a prototypal stage. The possible straightforward speed-up in this PR would be to avoid a full loop on all the trajectory hits making some assumption on standard patterns (only the first or first two hits are MTD). Unexpected more complex patterns are not used, but the code as it is now can be used for tagging them for possible further insight. Anyway I did not measure the potential time gain, but I guess it is small.

[fabiocos]

hold

investigating a crash observed in an extended test to monitor performances

[cmsbuild]

Pull request has been put on hold by @fabiocos
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

[fabiocos]

unhold

the failure was just due to an additional debug printout not present in the proposed branch

[fabiocos]

Measuring with the Timing service the time taken by TrackExtenderWithMTD on 100 events of 34634.0 (TTbar noPU), I see that the time is the same at the per-mil level with the default version of this PR, or whether I skip the loop on hits with the condition

if (ihitcount == 2 || (ihitcount == 1 && ietlcount == 0) ) { break; }

[srimanob]

Hi @fabiocos
Is this PR good for you now, or still have issues to answer?

[fabiocos]

@srimanob as far as I can see the PR works as expected, runs on 100 events without apparent issues, so I believe it is ready for integration. The only possible reason to wait is to get #33252 merged first, rerun tests on top of that and check the change on time and error per track. I doubt we may learn much more, but the effect of the update would be visible in a plot.

[srimanob]

OK Thanks, then I will wait a bit. If that can't converge quickly, I will propose to merge this first. Thanks @fabiocos

[fabiocos]

@srimanob we have agreed a number of updates to #33252 with @gsorrentino18 which will be deployed later today, then I hope we may merge it, and this will be important also for the validation of #33223 . I believe there is nothing wrong with this PR, but having a cross check with an improved validation tool can anyway give more confidence, as we are not in a special hurry for a pre-release build

[fabiocos]

please test

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-bf199f/13830/summary.html
COMMIT: 3647ff4
CMSSW: CMSSW_11_3_X_2021-03-29-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/33226/13830/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: 141 differences found in the comparisons
• DQMHistoTests: Total files compared: 37
• DQMHistoTests: Total histograms compared: 2640035
• DQMHistoTests: Total failures: 869
• DQMHistoTests: Total nulls: 1
• DQMHistoTests: Total successes: 2639143
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: -0.004 KiB( 36 files compared)
• DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
• Checked 155 log files, 37 edm output root files, 37 DQM output files
• TriggerResults: no differences found

[fabiocos]

The newly added plot of times associated with the tracks shows that this PR induces an increase in the number of tracks with smaller associated time uncertainties, deriving from the combination of two ETL hits. As so far the uncertainty is fixed at 25 ps, the average reduced uncertainty from the quadrature combination of two such measurements is ~ 18 ps:

[srimanob]

+Upgrade

[cmsbuild]

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)

[silviodonato]

+1