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

LST object build upgrade: change pT5 rz chi2 cut definition into helix approximation #40

Merged
merged 4 commits into from
Jun 19, 2024
Merged

LST object build upgrade: change pT5 rz chi2 cut definition into helix approximation #40

merged 4 commits into from
Jun 19, 2024

Conversation

YonsiG
Copy link

@YonsiG YonsiG commented Jun 11, 2024

PR description:

This PR is to upgrade the LST tracking algorithm. In the pT5 of LST object, we used to used linear approximation for its trajectory in r-z plane. However, a better approximation is that we can treat the pT5 tracks using helix approximation. Place the cut based on keeping 99% true tracks. Slides describing the algorithm can be found here. https://indico.cern.ch/event/1410985/contributions/5931017/attachments/2875400/5035406/helix%20approxi%20for%20pT5%20rzchi2%20new%20results%20versions.pdf

@YonsiG
Copy link
Author

YonsiG commented Jun 11, 2024

/run all

@ariostas
Copy link
Member

run all

Since this repo is a fork, it seems like membership is a bit weird so the CI didn't trigger. I might just need to hardcode usernames that can trigger the CI.

@VourMa
Copy link
Collaborator

VourMa commented Jun 12, 2024

Since this repo is a fork, it seems like membership is a bit weird so the CI didn't trigger. I might just need to hardcode usernames that can trigger the CI.

Just to note that Gavin's command seem to work normally, not sure if his username is explicitly in the sudo list.
Anyways, we can check and fix after you are back from vacations 😃

@YonsiG
Copy link
Author

YonsiG commented Jun 12, 2024

run all

Since this repo is a fork, it seems like membership is a bit weird so the CI didn't trigger. I might just need to hardcode usernames that can trigger the CI.

Should I directly push and create a CMSSW branch, not under my forked repo, so that the CI can be triggered? I think Gavin is pushing to cmssw branch.

@VourMa
Copy link
Collaborator

VourMa commented Jun 12, 2024

Since this repo is a fork, it seems like membership is a bit weird so the CI didn't trigger. I might just need to hardcode usernames that can trigger the CI

Ah, I see the problem now. SOrry, I misunderstood the initial comment, so this may make sense. However, there is also #44 (comment) which is not explained by this theory.

Should I directly push and create a CMSSW branch, not under my forked repo, so that the CI can be triggered? I think Gavin is pushing to cmssw branch.

I would say finish this PR in the branch that you have, let's not complicate ourselves. In any case, now you can produce your own validation plots. 😉

@slava77
Copy link

slava77 commented Jun 12, 2024

/run all

@GNiendorf
Copy link
Member

@YonsiG Could you please make a comment directly in the code saying what percentile you tuned the chi-squared values to? The reason I ask is because I noticed in other parts of the code slides were linked by other people but some of those links went dead, so all of that info got lost. Also, do you have the script somewhere that you used to tune these chi-squared values from the ntuple? If it's simple enough maybe we shouldn't include it, but I figured I should ask. I don't know what @slava77 and @VourMa think about this.

@slava77
Copy link

slava77 commented Jun 12, 2024

Also, do you have the script somewhere that you used to tune these chi-squared values from the ntuple? If it's simple enough maybe we shouldn't include it, but I figured I should ask. I don't know what @slava77 and @VourMa think about this.

We discussed this on Monday but postponed the conclusion until the Tuesday meeting and then apparently missed to check/discuss.
Let's get the analysis script available in a SegmentLinking repository.

@VourMa
Copy link
Collaborator

VourMa commented Jun 13, 2024

/run all

@VourMa
Copy link
Collaborator

VourMa commented Jun 13, 2024

@YonsiG The code checks failed, please run
scram b -j 12 code-checks >& c.log && scram b -j 12 code-format >& f.log
and apply the change when you commit again.

Copy link

The PR was built and ran successfully in standalone mode. Here are some of the comparison plots.

Efficiency vs pT comparison Efficiency vs eta comparison
Fake rate vs pT comparison Fake rate vs eta comparison
Duplicate rate vs pT comparison Duplicate rate vs eta comparison

The full set of validation and comparison plots can be found here.

Here is a timing comparison:

   Evt    Hits       MD       LS      T3       T5       pLS       pT5      pT3      TC       Reset    Event     Short             Rate
   avg     46.8    322.8    124.7     48.2     95.3    499.3    133.2    157.2    103.7      2.8    1534.0     987.9+/- 264.7     420.8   explicit_cache[s=4] (target branch)
   avg     44.1    326.1    122.5     48.1     97.8    551.7     89.8    163.3    124.2      2.3    1569.9     974.1+/- 255.1     429.4   explicit_cache[s=4] (this PR)

Copy link

The PR was built and ran successfully with CMSSW. Here are some plots.

OOTB All Tracks
Efficiency and fake rate vs pT, eta, and phi

The full set of validation and comparison plots can be found here.

@VourMa
Copy link
Collaborator

VourMa commented Jun 13, 2024

Based on the test output, doesn't it seem like this PR and the target branch have been swapped? @YonsiG could you verify with standalone plots run locally?
@ariostas Should we indeed push Yanxi's PR to the SegmentLinking repo and then make the PR again, i.e. could this be an issue with forks?

@slava77
Copy link

slava77 commented Jun 13, 2024

Based on the test output, doesn't it seem like this PR and the target branch have been swapped? @YonsiG could you verify with standalone plots run locally?

red (master) in the standalone looks the same as in other technical PRs.
It looks like something is off in this PR (inconsistent with SegmentLinking/TrackLooper#410 )

@ariostas
Copy link
Member

Should we indeed push Yanxi's PR to the SegmentLinking repo and then make the PR again, i.e. could this be an issue with forks?

It shouldn't matter that this is from Yanxi's fork. I'll take a look to see what's happening.

@YonsiG
Copy link
Author

YonsiG commented Jun 14, 2024

/run all

Copy link

The PR was built and ran successfully in standalone mode. Here are some of the comparison plots.

Efficiency vs pT comparison Efficiency vs eta comparison
Fake rate vs pT comparison Fake rate vs eta comparison
Duplicate rate vs pT comparison Duplicate rate vs eta comparison

The full set of validation and comparison plots can be found here.

Here is a timing comparison:

   Evt    Hits       MD       LS      T3       T5       pLS       pT5      pT3      TC       Reset    Event     Short             Rate
   avg     43.6    322.4    122.8     47.0     94.9    500.8    132.6    155.9    105.8      2.0    1527.8     983.5+/- 261.2     418.9   explicit_cache[s=4] (target branch)
   avg     43.7    324.7    122.3     49.2     95.5    500.7    119.1    160.2    105.3      2.0    1522.6     978.3+/- 263.0     416.4   explicit_cache[s=4] (this PR)

Copy link

The PR was built and ran successfully with CMSSW. Here are some plots.

OOTB All Tracks
Efficiency and fake rate vs pT, eta, and phi

The full set of validation and comparison plots can be found here.

Copy link
Collaborator

@VourMa VourMa left a comment

Choose a reason for hiding this comment

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

The PR now looks good to me, thanks @YonsiG! I am approving but I will merge a bit later in case anyone has any leftover comment.

I want to also clarify that merging this PR resolves #42.

@VourMa VourMa linked an issue Jun 19, 2024 that may be closed by this pull request
@VourMa VourMa merged commit d403d7a into SegmentLinking:CMSSW_14_1_0_pre3_LST_X_LSTCore_realfiles_batch1_devel Jun 19, 2024
@YonsiG
Copy link
Author

YonsiG commented Jun 20, 2024

Thank you @VourMa !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port SegmentLinking/TrackLooper#405 here
5 participants