Implement T3 r-z helix chi2 cuts #92

bucket420 · 2024-09-13T07:45:26Z

Summary:

This PR implements T3 r-z helix chi2 cuts, which replace the current linear approximation method. 2 MDs are used to build a helical trajectory, and the remaining MD is used to calculate the chi2. The T3s are divided into 25 regions according to the positions of the MDs, and the cuts are selected based on the ROC curves in each regions.

To get optimal performance, the linear cuts are kept for regions 9, 20-24, the middle MD is used to calculate the helix chi2 for regions 5 and 19, and the last MD is used to calculate the helix chi2 for all other regions.

Performance:

pT5: increased efficiency, slightly increase fake rate
T5: no significant changes in efficiency, increased fake rate
pT3: decreased efficiency, increased fake rate. Maybe due to cross cleaning?
TC: no significant changes in efficiency, increased fake rate at lower pt, decreased fake rate at higher pt
Linear breakdown: efficiency, fake rate
Helix breakdown: efficiency, fake rate

Link to slides.

slava77 · 2024-09-13T16:11:03Z

please update the target branch to CMSSW_14_1_0_pre3_LST_X_LSTCore_realfiles_batch1_devel

… into CMSSW_14_1_0_pre3_LST_X_LSTCore_realfiles_T3_rzchi2_final

bucket420 · 2024-09-18T00:28:11Z

/run all

ariostas · 2024-09-18T13:21:35Z

@bucket420 I added you so you can use the CI. If you leave another comment it should work

bucket420 · 2024-09-18T17:19:41Z

/run all

github-actions · 2024-09-18T17:34:43Z

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

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.5    323.3    114.6     45.6     90.6    499.1    112.1    148.6     99.2      2.4    1479.1     936.5+/- 247.4     405.1   explicit_cache[s=4] (target branch)
   avg     47.2    328.0    115.7     96.8    107.8    506.3    121.1    135.8    102.2      1.9    1562.8    1009.3+/- 261.0     424.1   explicit_cache[s=4] (this PR)

github-actions · 2024-09-18T17:56:52Z

There was a problem while building and running with CMSSW. The logs can be found here.

ariostas · 2024-09-18T18:37:04Z

There was a problem while building and running with CMSSW.

This is probably because the devel branch has not been updated to 14_2_0_pre1

VourMa

As a general comment, it would be great if the calculations applied here would be harmonized with the ones applied for T5: the logic is the same but it is applied in a different order. Even better, would it make sense to put some of the code here in functions and apply those, both here and in the T5 calculations, to simplify the code and make it clearer. It would also expose potential bugs and/or omissions in the different implementations.

RecoTracker/LSTCore/src/alpaka/Triplet.h

VourMa · 2024-09-26T12:10:36Z

Just to understand the future of this PR, do you intend to put things in functions and reuse them for the different objects? My request seems to become even more relevant in light of #97 (comment).

bucket420 · 2024-09-26T17:27:27Z

Yes, I do plan to refactor the code so that we can reuse functions for different objects. Just wondering if I should put the common functions in a new file or somewhere else? Also, would it be better to make a separate PR for that? Or should I keep working on this PR?

VourMa · 2024-09-26T19:30:13Z

Yes, I do plan to refactor the code so that we can reuse functions for different objects. Just wondering if I should put the common functions in a new file or somewhere else? Also, would it be better to make a separate PR for that? Or should I keep working on this PR?

Keep working on this PR would be fine for me.
As the code is right now, a lot of the common functions are in the Hit.h file but I don't think this is the best place. Either Kernels.h (would require a new include) or alpaka/Constants.h would work? Maybe people have a(nother) preference.

RecoTracker/LSTCore/src/alpaka/Triplet.h

slava77 · 2024-09-30T14:37:45Z

/run all

VourMa

Thank you for the updates, @bucket420! I think the majority of the comments have been dealt with. There are a few small follow-ups on still unresolved discussions.

By the way, I see that the syntax tests fail. Have you tried running scram b -j 12 code-checks >& c.log && scram b -j 12 code-format >& f.log and commit the proposed changes?
@ariostas will that work or there are issues with code checks?

RecoTracker/LSTCore/src/alpaka/Triplet.h

bucket420 · 2024-10-18T03:45:00Z

/run all

github-actions · 2024-10-18T03:59:12Z

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

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     44.0    321.6    116.6     47.9     90.2    544.9    110.5    149.4    104.6      2.7    1532.4     943.5+/- 249.3     419.5   explicit_cache[s=4] (target branch)
   avg     43.2    320.2    114.8     71.8    117.9    545.1    119.2    138.9    106.5      3.0    1580.4     992.1+/- 262.1     431.0   explicit_cache[s=4] (this PR)

github-actions · 2024-10-18T05:13:40Z

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

OOTB All Tracks

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

bucket420 · 2024-10-19T00:11:13Z

Thank you for the updates, @bucket420! I think the majority of the comments have been dealt with. There are a few small follow-ups on still unresolved discussions.

By the way, I see that the syntax tests fail. Have you tried running scram b -j 12 code-checks >& c.log && scram b -j 12 code-format >& f.log and commit the proposed changes? @ariostas will that work or there are issues with code checks?

I've run the command and committed the suggested change, but the static checks still failed

VourMa · 2024-10-20T17:08:33Z

/run checks

VourMa · 2024-10-20T17:25:43Z

I've run the command and committed the suggested change, but the static checks still failed

I took the liberty to try and fix the formatting issues. My hypothesis is that you were working on an older CMSSW version and the formatting changes were not compatible with the ones in CMSSW_14_2_0_pre1 which is our baseline now. I set up an CMSSW_14_2_0_pre1 area, checked out your branch there and used scram b code-format. Let's see if that worked.

Other than that, the PR seems to be ok from my side. I left a comment about having a presentation linked as documentation, and let's agree that this be added to the PR description, so that it doesn't block the merge.
@slava77 and @bucket420 could you clarify whether the last couple of comments have been successfully resolved and let me know, so that I can merge?

… into CMSSW_14_1_0_pre3_LST_X_LSTCore_realfiles_T3_rzchi2_final

VourMa · 2024-10-20T17:49:22Z

/run checks

VourMa · 2024-10-20T21:32:32Z

I took the liberty to try and fix the formatting issues. My hypothesis is that you were working on an older CMSSW version and the formatting changes were not compatible with the ones in CMSSW_14_2_0_pre1 which is our baseline now. I set up an CMSSW_14_2_0_pre1 area, checked out your branch there and used scram b code-format. Let's see if that worked.

Well, I failed. Pinging @ariostas again, for any ideas on the source of the issue.

slava77 · 2024-10-21T13:43:09Z

@slava77 and @bucket420 could you clarify whether the last couple of comments have been successfully resolved and let me know, so that I can merge?

I think that my question/comment about variable naming was clarified

slava77 · 2024-10-21T13:44:31Z

My hypothesis is that you were working on an older CMSSW version and the formatting changes

yes, CI is further ahead than this topic and target branch (we are missing batch6 (and 5?) in the devel branch)

VourMa · 2024-10-21T13:50:30Z

yes, CI is further ahead than this topic and target branch (we are missing batch6 (and 5?) in the devel branch)

Ok, then I propose that we merge even with the static checks not passed.

Unless there is any disagreement, I will merge the PR as is at the end of my day (~4h).

slava77 · 2024-10-21T14:18:49Z

yes, CI is further ahead than this topic and target branch (we are missing batch6 (and 5?) in the devel branch)

Ok, then I propose that we merge even with the static checks not passed.

Unless there is any disagreement, I will merge the PR as is at the end of my day (~4h).

I merged PRs in devel recently after checking explicitly in the code check logs that the part to be modified was not modified by the PR

VourMa · 2024-10-21T14:47:56Z

yes, CI is further ahead than this topic and target branch (we are missing batch6 (and 5?) in the devel branch)

Ok, then I propose that we merge even with the static checks not passed.
Unless there is any disagreement, I will merge the PR as is at the end of my day (~4h).

I merged PRs in devel recently after checking explicitly in the code check logs that the part to be modified was not modified by the PR

Indeed, this is the case. As you may have noticed in my last commits, I ended up chasing differences in the standalone folder, which is not what this PR is changing.

slava77 · 2024-10-25T23:21:56Z

RecoTracker/LSTCore/standalone/code/rooutil/cxxopts.h

@@ -238,8 +238,7 @@ namespace cxxopts {
  class Option_syntax_exception : public OptionParseException {
  public:
    Option_syntax_exception(const std::string& text)
-        : OptionParseException("Argument " + LQUOTE + text + RQUOTE + " starts with a - but has incorrect syntax") {
-    }
+        : OptionParseException(u8"Argument " + LQUOTE + text + RQUOTE + u8" starts with a - but has incorrect syntax") {}


@VourMa
it looks like this broke the standalone in devel

Seems like it, even though I am not sure why this change was proposed by the checks. Anyways, trying to fix the checks was a bigger failure that initially thought, sorry about that.

Let me make a standalone PR to fix that.

bucket420 added 3 commits September 12, 2024 14:35

Add T3 helix chi2 cuts

0ef4e32

refactor code

cbf6efe

Remove linear cuts

abd642d

bucket420 changed the base branch from CMSSW_14_1_0_pre3_LST_X_LSTCore_realfiles to CMSSW_14_1_0_pre3_LST_X_LSTCore_realfiles_batch1_devel September 13, 2024 16:16

bucket420 marked this pull request as ready for review September 16, 2024 20:54

Merge branch 'CMSSW_14_1_0_pre3_LST_X_LSTCore_realfiles_batch1_devel'…

c8d42dc

… into CMSSW_14_1_0_pre3_LST_X_LSTCore_realfiles_T3_rzchi2_final

bucket420 marked this pull request as draft September 17, 2024 17:00

Resolve conflicts

e10e950

bucket420 marked this pull request as ready for review September 18, 2024 00:27

GNiendorf mentioned this pull request Sep 20, 2024

T5 DNN Cleanup + Training Scripts + Eta/pT Dependent Cut #61

Merged

VourMa reviewed Sep 21, 2024

View reviewed changes

bucket420 added 3 commits September 23, 2024 13:04

Address comments

1d0a541

Address comments

ff8446c

Avoid using trig functions and correct linear cuts

a194521

GNiendorf reviewed Sep 25, 2024

View reviewed changes

RecoTracker/LSTCore/src/alpaka/Triplet.h Outdated Show resolved Hide resolved

slava77 reviewed Sep 25, 2024

View reviewed changes

RecoTracker/LSTCore/src/alpaka/Triplet.h Outdated Show resolved Hide resolved

slava77 reviewed Sep 25, 2024

View reviewed changes

RecoTracker/LSTCore/src/alpaka/Triplet.h Outdated Show resolved Hide resolved

Simplify charge calculation and update variable names

abd861f

YonsiG reviewed Sep 30, 2024

View reviewed changes

RecoTracker/LSTCore/src/alpaka/Triplet.h Show resolved Hide resolved

VourMa reviewed Oct 10, 2024

View reviewed changes

VourMa mentioned this pull request Oct 10, 2024

Put code for r-z helix χ^2 cuts in functions and use those for both the T3 and T5 calculations #102

Open

bucket420 added 2 commits October 14, 2024 22:35

Update comments and revert unnecessary change

b851ef9

Use edm::isNotFinite to check for NaNs and fix variable names

629f6b0

slava77 reviewed Oct 17, 2024

View reviewed changes

RecoTracker/LSTCore/src/alpaka/Triplet.h Outdated Show resolved Hide resolved

slava77 reviewed Oct 17, 2024

View reviewed changes

RecoTracker/LSTCore/src/alpaka/Triplet.h Outdated Show resolved Hide resolved

bucket420 added 2 commits October 17, 2024 20:13

Fix NaN handling

0477c83

Add link to PR and fix syntax

1ab92d0

Fix formatting issues

7dbe3a1

VourMa added 2 commits October 20, 2024 10:45

One more formatting fix

f7a218f

Merge branch 'CMSSW_14_1_0_pre3_LST_X_LSTCore_realfiles_batch1_devel'…

992947d

… into CMSSW_14_1_0_pre3_LST_X_LSTCore_realfiles_T3_rzchi2_final

VourMa approved these changes Oct 21, 2024

View reviewed changes

VourMa merged commit 7a2c422 into CMSSW_14_1_0_pre3_LST_X_LSTCore_realfiles_batch1_devel Oct 21, 2024
1 check failed

slava77 reviewed Oct 25, 2024

View reviewed changes

VourMa mentioned this pull request Oct 26, 2024

Fix wrong code format #108

Merged

ariostas mentioned this pull request Nov 6, 2024

Devel changes that need to be rebased #117

Open

15 tasks

ariostas mentioned this pull request Nov 25, 2024

Implement T3 r-z helix chi2 cuts (rebase PR92) #131

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement T3 r-z helix chi2 cuts #92

Implement T3 r-z helix chi2 cuts #92

bucket420 commented Sep 13, 2024 •

edited by VourMa

Loading

slava77 commented Sep 13, 2024

bucket420 commented Sep 18, 2024

ariostas commented Sep 18, 2024

bucket420 commented Sep 18, 2024

github-actions bot commented Sep 18, 2024

github-actions bot commented Sep 18, 2024

ariostas commented Sep 18, 2024

VourMa left a comment

VourMa commented Sep 26, 2024

bucket420 commented Sep 26, 2024

VourMa commented Sep 26, 2024

slava77 commented Sep 30, 2024

VourMa left a comment

bucket420 commented Oct 18, 2024

github-actions bot commented Oct 18, 2024

github-actions bot commented Oct 18, 2024

bucket420 commented Oct 19, 2024

VourMa commented Oct 20, 2024

VourMa commented Oct 20, 2024

VourMa commented Oct 20, 2024

VourMa commented Oct 20, 2024

slava77 commented Oct 21, 2024

slava77 commented Oct 21, 2024 •

edited

Loading

VourMa commented Oct 21, 2024

slava77 commented Oct 21, 2024

VourMa commented Oct 21, 2024

slava77 Oct 25, 2024

VourMa Oct 26, 2024

Implement T3 r-z helix chi2 cuts #92

Implement T3 r-z helix chi2 cuts #92

Conversation

bucket420 commented Sep 13, 2024 • edited by VourMa Loading

Summary:

Performance:

slava77 commented Sep 13, 2024

bucket420 commented Sep 18, 2024

ariostas commented Sep 18, 2024

bucket420 commented Sep 18, 2024

github-actions bot commented Sep 18, 2024

github-actions bot commented Sep 18, 2024

ariostas commented Sep 18, 2024

VourMa left a comment

Choose a reason for hiding this comment

VourMa commented Sep 26, 2024

bucket420 commented Sep 26, 2024

VourMa commented Sep 26, 2024

slava77 commented Sep 30, 2024

VourMa left a comment

Choose a reason for hiding this comment

bucket420 commented Oct 18, 2024

github-actions bot commented Oct 18, 2024

github-actions bot commented Oct 18, 2024

bucket420 commented Oct 19, 2024

VourMa commented Oct 20, 2024

VourMa commented Oct 20, 2024

VourMa commented Oct 20, 2024

VourMa commented Oct 20, 2024

slava77 commented Oct 21, 2024

slava77 commented Oct 21, 2024 • edited Loading

VourMa commented Oct 21, 2024

slava77 commented Oct 21, 2024

VourMa commented Oct 21, 2024

slava77 Oct 25, 2024

Choose a reason for hiding this comment

VourMa Oct 26, 2024

Choose a reason for hiding this comment

bucket420 commented Sep 13, 2024 •

edited by VourMa

Loading

slava77 commented Oct 21, 2024 •

edited

Loading