Update CkfTrajectoryBuilder to handle hitless seeds

[BenjaminRS]

Making the pull request after discussing with Tracking POG and giving the presentation during the Tracking meeting on the 17th November (as discussed and linked in the code comments).

[cmsbuild]

A new Pull Request was created by @BenjaminRS (Benjamin Radburn-Smith) for CMSSW_7_3_X.

Update CkfTrajectoryBuilder to handle hitless seeds

It involves the following packages:

RecoTracker/CkfPattern

@cmsbuild, @nclopezo, @StoyanStoynev, @slava77 can you please review it and eventually sign? Thanks.
@ghellwig, @makortel, @GiacomoSguazzoni, @rovere, @VinInn, @appeltel, @gpetruc, @cerati, @dgulhan, @venturia this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
@nclopezo you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

[StoyanStoynev]

@cmsbuild please test

[cmsbuild]

+1
Tested at: 5cb6e62
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6566/1104/summary.html

[cmsbuild]

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

[rovere]

Hi all,
there are still few pending issues and requests that the Tracking POG would
like to have solved before integrating this PR. Is it possible to keep this
on hold?

Ciao,
M.

On Mon Nov 24 2014 at 2:59:56 PM cmsbuild notifications@github.com wrote:

Comparison is ready

https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6566/1104/summary.html

—
Reply to this email directly or view it on GitHub
#6566 (comment).

[StoyanStoynev]

Do we have more on this by now?

[BenjaminRS]

I do not think there are any pending issues/requests from Tracking POG on this PR. Is this correct?
Benjamin

[StoyanStoynev]

@rovere

[rovere]

Here I am, and sorry for the delay.
To answer to Benjamin's question: I still would like to see a full porting
of iter10@HLT, seeding, PR and track-building, and have a clear proof that
the seedless approach you want to use is indeed better, since so far I saw
no proof of it.
The changes in the code itself are relatively small and we do not have
anything against them. But we cannot approve code in release that adds
another special treatment w/o a proof that what already is in release
cannot cover the same use case. So code-wise we could give our +1, from the
usefulness and opportunity to have this in, I still have my -1.
And apparently no other piece of code in CMSSW is crashing due to this
"unsupported" seedless tracking in the official tracking code: so there is
no other use-case that this PR will fix, apart from the one we are
discussing right now.

Benjamin, do not get me wrong, I'm not a bad guy. As I said several times,
I just want to have some real proof that the use case you would like to
have covered in the official Tracking code of CMSSW is indeed not covered
by something that is already available.

For the sake of the discussion, I would also like to include the Muon POG
conveners (@abbiendi @trocino @battibass) , since if we end up approving
this PR to support the use case we are talking about, the tracking POG has
no handle to control and assess the quality of the results, since, as I
said, no other piece of tracking code is depending on this. And since the
final outcome of this is a Muon, it will be Muon POG responsibility to
periodically check for the goodness of this.

Ciao,
M.

On Wed Nov 26 2014 at 10:11:04 AM StoyanStoynev notifications@github.com
wrote:

@rovere https://github.com/rovere

—
Reply to this email directly or view it on GitHub
#6566 (comment).

[StoyanStoynev]

I think this PR should be closed at this point and discussion moved to #6623. At the same time I don't see any progress or support now so I would think even #6623 should be closed until there is a resolution and clarity.

[BenjaminRS]

Hi Marco,

I have just come back from my holidays. As I was trying to point out in the offline thread we already use the hitless seed case as part of our OIState algorithm. It was handled by the MuonCkfTrajectoryBuilder class, so indeed the use case is already handled by CMSSW. But we wanted to get rid of the duplicated code and increase maintainability by removing that class and making this modification in the inherited class CkfTrajectoryBuilder.

It has been a conceptual belief that having the capacity to use hitless seeds would be beneficial as it would, among other things, stop biases when adding the hit and decreasing the time taken by the seeding. I believe this is why the use case was made and a separate CkfTrajectoryBuilder was made for Muons. This was done so before my time (and I have been told from the start of CMS). We use OIState as the first algorithm out of 3 as part of the cascade logic for Muon HLT L3 reconstruction. It tends to be very fast and in some cases more efficient than OIHit ([1] slide 16), which uses the hit based information when making seeds.

However, I do not have the direct proof you ask for at hand (using latest tracking code). I can work on it this week, but I do not know if this fits in with the timescales of Pull Requests.

Thanks,
Benjamin
[1] https://indico.cern.ch/event/297781/contribution/3/material/slides/0.pdf

[StoyanStoynev]

If this would fall into "substantial tracking changes" the deadline would be in about a week:
https://twiki.cern.ch/twiki/bin/view/CMS/CMSSW_7_4_0
Probably it is not the case but whatever is the case there should be agreement on that in advance (could discuss it a bit in the next RECO meeting or here/else). Also I didn't hear from the MUON POG - as I understand they should be supporting it in a longer run (@abbiendi @trocino @battibass) .

[battibass]

Hi,

For what concerns the use case of this PR itself, Benjamin has already commented well on the usefulness (muon HLT wise) of the change.
I would not consider it a substantial tracking change, the performance of the present tracking sequences should be unaffected, simply it would add to a new version of the muon L3 code, capabilities that are available in the present L3, but reducing the amount of code duplication.

For what concerns assessing the quality of the results, whenever the switch to the new muon HLT L3 code will happen, the changes will be monitored as part of our validation of L3 tracks and L3 seeds on a pre release by pre release basis ( == release validation of this new feature will be completely on the Muon POG).

More in depth studies will come (based both on MC and data) before (and right after) the deployment of the new L3 code, but after that, only the overall muon HLT performance will be looked at in real data case no problems (with efficiency or rate) will arise.

Marco (@rovere), Stoyan (@StoyanStoynev) does this answer your questions?

(putting the comment in this PR thread as here is where the questions were asked and adding @HuguesBrun)

[StoyanStoynev]

As long as Marco (Tracking POG) doesn't object, and his concerns are addressed, we can go forward. But we are still talking about 74X, right?

[rovere]

Hi Benjamin, all,
I understand that hitless seeding has been there in CMSSW since ages, but
so far it was not part of tracking POG code and was maintained basically by
the Muon HLT people. What you are asking for is to have it included in
standard tracking code, of which we are responsible. What I asked for is to
go from conceptual believes to a real proof that what we are using for the
Offline Reconstruction to recover muon efficiency (doing more or less
exactly the same logical steps you are implementing at HLT) is not good
enough and, in case, also to understand why it is not good@HLT (poorer
efficiency, timing out of control, fake rate, any combination of the
previous, any other reason?). As Giuseppe pointed out at some point, there
has been quite a strong effort from our side to try and bring Tracking code
@HLT as similar as possible to the offline one, modulo mandatory changes
mainly driven by timing considerations. I would try to go in this very same
direction also in this case.

More into the specific, I would definitely not consider this, in any
case, a substantial tracking change at all, and I would not apply the
deadline of Dec 8th for this. 73 is officially closed, hence I would go for
74X release: if a back-port will be needed, I'd argue that, due to the
extremely small code changes we are talking about, we can afford to have it
done rather easily.

Again, it's nothing related to the proposed code changes, it's more on the
logical approach to this PR.

Thanks to everybody for you understanding and hard work on this: do not
believe it's not been appreciated.

Ciao,
M.

On Tue Dec 02 2014 at 9:05:58 AM StoyanStoynev notifications@github.com
wrote:

As long as Marco (Tracking POG) doesn't object, and his concerns are
addressed, we can go forward. But we are still talking about 74X, right?

—
Reply to this email directly or view it on GitHub
#6566 (comment).

[StoyanStoynev]

@rovere can you discuss this PR in the next POG meeting?

[StoyanStoynev]

@BenjaminRS (for the comment above)

[rovere]

Ciao @StoyanStoynev
what exactly do you suggest to talk about in the next Tracking POG Meeting (next year)? I think we expressed clearly what we think and what we would like to have checked by the proponent of this PR.

Ciao,
M.

[StoyanStoynev]

@rovere Sorry, I thought the meeting was next Monday...
It is just that the arguments are going back-and-forth by mail/github and the PR is stuck. I thought talking in person would be helpful. Other than that I agree with your requests entirely.

[davidlange6]

closing this - its for 74x where there is already a PR