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.

Sign up for GitHub

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

Jump to bottom

L1 tk dev 12 0 0 pre4 hph before rebase (not to be merged) #96

Closed
wants to merge 11 commits into from
Closed

L1 tk dev 12 0 0 pre4 hph before rebase (not to be merged) #96

wants to merge 11 commits into from

Conversation

Jingyan95
Copy link

@Jingyan95 Jingyan95 commented Oct 16, 2021 •
edited
Loading

The crash described in PR#94 is fixed with this new PR. I will create a separate PR to include changes made to ntuple maker that is requested by Chris.

@tomalin
Copy link
Collaborator

tomalin commented Oct 19, 2021

I've verified L1TrackNtupleMaker_cfg.py runs and gives unchanged results.

@tomalin
Copy link
Collaborator

tomalin commented Nov 22, 2021

@tschuh points out that for the new KF, the class TrackerTFP/interface/LayerEncoding.h determines the KF layer encoding, so you should check if calling this would significantly simplify the HitPatternHelper.

@tomalin
Copy link
Collaborator

tomalin commented Nov 22, 2021

I advise making clear in comments which part of HitPatternHelper are only needed to handle the case of the Old KF. (And also say somewhere in the code what is meant by Old KF).

@tomalin tomalin mentioned this pull request Nov 22, 2021
Closed
@Jingyan95
Copy link
Author

@tschuh points out that for the new KF, the class TrackerTFP/interface/LayerEncoding.h determines the KF layer encoding, so you should check if calling this would significantly simplify the HitPatternHelper.

I am a bit hesitant at this one because:

  1. TrackerTFP package already depends on TrackTrigger to set up the configuration. Is it really a good idea to ask TrackTrigger to use a product from TrackerTFP ?
  2. Also that laye-encoding function written by Thomas is binned by eta/cot/z0. I find it unnecessary for HitPatternHelper, which has access to accurate track parameters.

@tomalin tomalin changed the title L1 tk dev 12 0 0 pre4 hph L1 tk dev 12 0 0 pre4 hph before rebase (not to be merged) Dec 16, 2021
@tomalin tomalin requested a review from skinnari February 5, 2022 11:46
skinnari
skinnari reviewed Apr 5, 2022
View reviewed changes
L1Trigger/TrackTrigger/src/HitPatternHelper.cc Outdated
(z0_ - sm.z() + sm.r() * (cot_ + deltaTanL_ / 2)) / (sm.cosTilt() - sm.sinTilt() * (cot_ + deltaTanL_ / 2));
double d_m =
(z0_ - sm.z() + sm.r() * (cot_ - deltaTanL_ / 2)) / (sm.cosTilt() - sm.sinTilt() * (cot_ - deltaTanL_ / 2));
// if (!(abs(d_p) < sm.numColumns() * sm.pitchCol() / 2. && abs(d_m) < sm.numColumns() * sm.pitchCol() / 2.))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please delete the out-commented lines of code (unless there is a particular reason they are kept?)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@skinnari skinnari mentioned this pull request Apr 14, 2022
Merged
@skinnari
Copy link

merged via #146 so closing this.

@skinnari skinnari closed this Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomalin tomalin tomalin left review comments

@skinnari skinnari skinnari left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Jingyan95 @tomalin @skinnari