Support hitless seeds in GroupedCkfTrajectory

[folguera]

Enable hitless seed compatibility to the GroupedCkfTrajectory so it can be used in the muon HLT reconstruction. Change is transparent to any user of the GroupedCkfTrajectory.

Removed unused function in BaseCkfTrajectoryBuilder. Will need back port to 92X.

[cmsbuild]

The code-checks are being triggered in jenkins.

[cmsbuild]

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20556/762

Code check has found code style and quality issues which could be resolved by applying a patch in https://cmssdt.cern.ch/SDT/code-checks/PR-20556/762/git-diff.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/PR-20556/762/git-diff.patch | patch -p1

You can run scram build code-checks to apply code checks directly (this will soon be required for PRs to be considered)

[cmsbuild]

The code-checks are being triggered in jenkins.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20556/763

[cmsbuild]

A new Pull Request was created by @folguera (Santiago Folgueras) for master.

It involves the following packages:

RecoTracker/CkfPattern

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@ghellwig, @makortel, @felicepantaleo, @GiacomoSguazzoni, @rovere, @VinInn, @mschrode, @gpetruc, @ebrondol, @dgulhan this is something you requested to watch as well.
@davidlange6, @slava77 you are the release manager for this.

cms-bot commands are listed here

[perrotta]

@folguera , this PR has already conflicts to be solved: please fix them

[cmsbuild]

The code-checks are being triggered in jenkins.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20556/764

[cmsbuild]

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

[cmsbuild]

+1
Tested at: 7fee5f3
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20556/23034/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
7fee5f3
aa837d3
a3917b1
1804adc
09e02e8
3f53835
ca0b101
dbc259d
d02b532
390ffe7
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20556/23034/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20556/23034/git-merge-result

[cmsbuild]

Comparison job queued.

[cmsbuild]

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20556/23034/summary.html

Comparison Summary:

• No significant changes to the logs found
• Reco comparison results: 0 differences found in the comparisons
• DQMHistoTests: Total files compared: 26
• DQMHistoTests: Total histograms compared: 2644232
• DQMHistoTests: Total failures: 385
• DQMHistoTests: Total nulls: 345
• DQMHistoTests: Total successes: 2643313
• DQMHistoTests: Total skipped: 189
• DQMHistoTests: Total Missing objects: 0
• Checked 107 log files, 14 edm output root files, 26 DQM output files

[perrotta]

@folguera : just in order to understand how this works now, in line
https://github.com/folguera/cmssw/blob/7fee5f3b5c847d25b1430cf40ae5f0ff3a04fbdb/RecoTracker/CkfPattern/src/BaseCkfTrajectoryBuilder.cc#L221
the traj.direction() is used as argument of StateAndLayers(...) when traj.empty().

How can it work? It does work, indeed: then I imagine that such a direction is not needed. Could you please point me to where the code deals with those missing directions?

[folguera]

Hi @perrotta,

I can investigate how this is dealt with, but I don't know the details about this. I just simply changed that line that was also used by the older CkfTrajectoryBuilder and that we were using for the muon HLT reconstruction.

In our case the temp trajectory has no measurement (hitless) but a direction of propagation, so I'm not sure that you don't have the direction in any case. Am I making sense?

[VinInn]

@perotta, what makes you thinking that traj.direction() has no meaning when traj.empty()?

[perrotta]

Ok, that direction is indeed just an enum, which keeps its meaning even if traj.empty(), see DataFormats/TrajectorySeed/interface/PropagationDirection.h: this is basically the answer to my question

[perrotta]

You write: "Change is transparent to any user of the GroupedCkfTrajectory". This means that currently all user never end up with traj.empty(), otherwise there should be some different behaviour for them.
The question is the following: is there any parameter or instruction in the code that prevents empty trajectories now, that will be switched off when you'll use the code for hitless seeds?

[VinInn]

@perotta,
is the opposite. currently a "user" with a hitless seed will produce a crash (in the first version there was even an assert)
now it will go through

Essentially this re-establishes the behaviour prior to Run2

btw there are no "user" of GroupedCkfTrajectory, only TRK-POG

[perrotta]

(Slowly trying to understand how this code works... thank you @VinInn)

Well, currently in case of a hitless seed the method returns an uninitialized StateAndLayers(), which does not obviously imply to me that there will be a crash: does it?

If it does not crash, then it is not true that "Change is transparent to any user of the GroupedCkfTrajectory", unless there is some parameter or part of the code that prevents it.

Then, simplifying, my question is still: it never happened that some hitless seed entered that method AND the code did not crash? IOW: it is true that "Change is transparent to any user of the GroupedCkfTrajectory"?

[VinInn]

On 21 Sep, 2017, at 3:49 PM, perrotta ***@***.***> wrote: (Slowly trying to understand how this code works... thank you @VinInn) Well, currently in case of a hitless seed the method returns an uninitialized StateAndLayers(), which does not obviously imply to me that there will be a crash: does it?

well it will fill the output of "CkfPattern: empty traj. Skipping." and we never been reported such a case

If it does not crash, then it is not true that "Change is transparent to any user of the GroupedCkfTrajectory", unless there is some parameter or part of the code that prevents it. Then, simplifying, my question is still: it never happened that some hitless seed entered that method AND the code did not crash? IOW: it is true that "Change is transparent to any user of the GroupedCkfTrajectory"?

Are you pretending a first principle proof? Just be happy that you do not see any regression. TRK-POG is happy

[perrotta]

+1

• It makes GroupedCkfTrajectory compatible to hit-less seeds, as it is already in CkfTrajectoryBuilder
• No visible regression on jenkins test results
• Not obvious whether it can affect also current use cases, but if so it is only with a negligible rate (it should have issued warning messages that were never noticed so far), and in any case it is endorsed by tracking POG, see Support hitless seeds in GroupedCkfTrajectory #20556 (comment)

[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. @davidlange6, @slava77, @smuzaffar (and backports should be raised in the release meeting by the corresponding L2)

[davidlange6]

are these changes thought to be part of a non-reproducibility problem in dqm?
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_9_4_X_2017-09-17-2300+20556/22549/10042.0_ZMM_13+ZMM_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+ALCAFull_2017+HARVESTFull_2017/PixelPhase1V.html

[VinInn]

PixelPhase1 sim w/o any effect on reco?
yes.... (already reported several time)

[davidlange6]

It seems that way - sigh - would be great not to have that problem as we've beaten down other non-reproducibility and nan issues affecting the comparisons

[davidlange6]

+1