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

Update CkfTrajectoryBuilder to handle hitless seeds #6623

Merged

Conversation

cmsbuild
Copy link
Contributor

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).
Automatically ported from CMSSW_7_3_X #6566

@cmsbuild
Copy link
Contributor Author

A new Pull Request was created by @cmsbuild for CMSSW_7_4_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
Copy link
Contributor

+1
Latest commit marked as "Tests OK" in CMSSW_7_3_X

@cerati
Copy link
Contributor

cerati commented Nov 26, 2014

Does this mean you are closing #6566 (still being discussed)?

@StoyanStoynev
Copy link
Contributor

Not yet supported by Tracking and/or Muon POG. The status is unclear to me.

@davidlange6
Copy link
Contributor

Is this PR obsolete?

@StoyanStoynev
Copy link
Contributor

It is not but action was requested for #6566 (same but in 73X).

@BenjaminRS
Copy link
Contributor

Hi Marco (@rovere),

Sorry for the delay with the tests. I brought the OutsideInMuonSeeder online and tested it compared to the seeder I created. The problem is the timing. Testing on a WMnuNu sample with PU: using the normal configuration (3 hits on 3 layers) we can get 94% efficiency but it takes 4.7 ms for the L3 reconstruction. Using the fastest possible configuration of hits (1 hit on 1 layer) gives 63% efficiency in 2.2 ms. Using the hitless seeds I can get 92% efficiency in 2.1 ms. From our studies we wish to have hitless seeds available.

From our view we don't mind having a new class inherit from CkfTrajectoryBuilder and making the modifications there which would then come under Muon POG control. We just thought the code duplication could be minimised by making the modification directly in CkfTrajectoryBuilder.

Thanks,
Benjamin

@slava77
Copy link
Contributor

slava77 commented Dec 24, 2014

-1

@BenjaminRS
From the last message it sounds like this PR can be closed.
#7003 seemed to be the target customer, but it apparently lived without it.
Let me know if I missed something.

@slava77
Copy link
Contributor

slava77 commented Jan 9, 2015

ok, it sounds like the situation hasn't changed since about 6 weeks ago (also discussion in #6566).

If we are going to stay with muon-specific seeding (not using tools used in iterative tracking),
then as I understand tracking POG preference, this muon-specific code should be implemented in a muon-specific class/builder.

@VinInn
Copy link
Contributor

VinInn commented Jan 16, 2015

Dear all, after discussion in the Tracking-POG it has been decided that this PR represents the less problematic solution to the issue.
Please reopen an re-sync this PR.
We will come back to an optimal solution in the context of a re-design of base tracking pattern-recognition that will include also requirements coming from phase1 and phase2

@slava77
Copy link
Contributor

slava77 commented Jan 16, 2015

OK, I'm looking forward for an open/new PR with this feature.

@slava77
Copy link
Contributor

slava77 commented Jan 17, 2015

@davidlange6
David, please reopen this.
It appears to still merge and compile fine.

@davidlange6 davidlange6 reopened this Jan 17, 2015
@davidlange6
Copy link
Contributor

Done.

@slava77
Copy link
Contributor

slava77 commented Jan 19, 2015

@cmsbuild please test

the old test area is long gone

@cmsbuild
Copy link
Contributor Author

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor Author

@cmsbuild
Copy link
Contributor Author

@slava77
Copy link
Contributor

slava77 commented Jan 20, 2015

+1

for #6623 5cb6e62
code eventually OK'd
the jenkins tests pass (runs and no differences appear)

@cmsbuild
Copy link
Contributor Author

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_4_X IBs unless changes (tests are also fine). This pull request will be automatically merged.

cmsbuild added a commit that referenced this pull request Jan 20, 2015
…ation

Update CkfTrajectoryBuilder to handle hitless seeds
@cmsbuild cmsbuild merged commit 59ba338 into cms-sw:CMSSW_7_4_X Jan 20, 2015
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

8 participants