Total removal of sdl/SDL #72

VourMa · 2024-07-30T15:45:08Z

Proposing the total removal of "sdl" or "SDL" from our code. This means:

Renaming of some variables in the main part of the code (done in 961d01e, transparent change)
Renaming the remaining standalone code, including directories, scripts, efficiency code and makefiles (done in d378a59). This change is more complicated, as it changes some of the commands, e.g. sdl_make_tracklooper -> lst_make_tracklooper for compilation. Due to that, I expect it to require changes in the standalone CI. The code compiles and runs fine locally.

slava77 · 2024-07-30T16:38:33Z

RecoTracker/LSTCore/src/alpaka/PixelTriplet.h

-    const float sdlPVoff = 0.1f / rt_OutLo;
-    sdlCut = alpha1GeV_OutLo + alpaka::math::sqrt(acc, sdlMuls2 + sdlPVoff * sdlPVoff);
+    const float lstPVoff = 0.1f / rt_OutLo;
+    lstCut = alpha1GeV_OutLo + alpaka::math::sqrt(acc, lstMuls2 + lstPVoff * lstPVoff);


in this and a few other cases perhaps lst or sdl can be dropped completely and where appropriate replaced by a more clear name.
Here dPhiCut is more appropriate.

in the above muls2 or below thetaMuls2 would be clear enough

Sure, I can change these.

slava77 · 2024-07-30T16:44:20Z

RecoTracker/LSTCore/standalone/bin/sdl_cpu

this file shouldn't be here (not even in the history)

Oops, my bad. I will fix it with a force push while dealing with the rest of the comments.

slava77 · 2024-07-30T16:46:38Z

RecoTracker/LSTCore/standalone/setup_hpg.sh

was removal of this intentional?

Yes. This file is since long broken, as it is not maintained. We can discuss whether it is needed (see also the relevant SegmentLinking/TrackLooper#412 (comment))

VourMa · 2024-07-30T18:35:56Z

@ariostas When you get the time, could you check the updates needed for the standalone CI implied by this PR? Does the PR look ok and easily "accommodatable"?

Not to overcomplicate ourselves for now, we can run the CMSSW CI to verify the performance, and I made sure that the standalone workflow runs successfully to the end locally (plots).

slava77 · 2024-07-30T20:02:13Z

/run all

slava77 · 2024-07-30T20:02:38Z

/run all

let's see what actually breaks

github-actions · 2024-07-30T20:07:00Z

There was a problem while building and running in standalone mode. The logs can be found here.

slava77 · 2024-07-30T20:15:07Z

I received also this for the linter job:
https://github.com/SegmentLinking/cmssw/actions/runs/10168947805
Error: Input required and not supplied: token

But then the PR shows that static checks are in progress.
@ariostas is this just new noise from the CI that it's sending notices while it's waiting for another job to complete?

github-actions · 2024-07-30T22:10:33Z

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.

ariostas · 2024-07-31T13:19:52Z

When you get the time, could you check the updates needed for the standalone CI implied by this PR? Does the PR look ok and easily "accommodatable"?

Yeah, I'll work on that.

is this just new noise from the CI that it's sending notices while it's waiting for another job to complete?

I think what happened was that your second comment also included the "run all" string, so it cancelled the previous jobs and started new ones

ariostas · 2024-07-31T15:07:02Z

I think it should work now.

/run standalone

github-actions · 2024-07-31T15:12:41Z

There was a problem while building and running in standalone mode. The logs can be found here.

ariostas · 2024-07-31T15:33:50Z

Second try.

/run standalone

github-actions · 2024-07-31T15:46:18Z

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     47.0    322.4    117.2     47.8     92.5    494.6    125.6    145.8    101.7      2.4    1496.8     955.3+/- 251.9     409.3   explicit_cache[s=4] (target branch)
   avg     47.3    323.8    117.7     46.7     92.5    494.5    125.4    145.7    102.3      2.6    1498.5     956.7+/- 249.2     409.4   explicit_cache[s=4] (this PR)

slava77 · 2024-07-31T16:37:34Z

/run all

github-actions · 2024-07-31T16:50:56Z

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     47.4    321.2    118.5     48.6     92.2    494.7    126.2    145.6    100.7      4.3    1499.3     957.3+/- 248.6     410.7   explicit_cache[s=4] (target branch)
   avg     47.0    325.1    119.3     48.2     93.7    495.3    126.7    147.8    101.4      1.9    1506.4     964.1+/- 253.2     411.7   explicit_cache[s=4] (this PR)

github-actions · 2024-07-31T18:01:44Z

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.

Remove sdl from variable names

961d01e

slava77 reviewed Jul 30, 2024

View reviewed changes

Removal of sdl/SDL from all standalone code

d378a59

VourMa force-pushed the removalOfSDL branch from f2b01dd to d378a59 Compare July 30, 2024 17:34

VourMa added 2 commits July 30, 2024 10:57

Simplification of variable naming

67c2376

Code checks and format

c3f41a7

VourMa changed the title ~~[RFC] Total removal of sdl/SDL~~ Total removal of sdl/SDL Jul 30, 2024

VourMa marked this pull request as ready for review July 30, 2024 18:29

Remove forgotten flag

a076afb

VourMa mentioned this pull request Jul 31, 2024

List of unresolved comments (proposal to deal with them in follow up PRs after #45117) #75

Open

25 tasks

slava77 approved these changes Jul 31, 2024

View reviewed changes

slava77 merged commit ecd8e3e into CMSSW_14_1_0_pre3_LST_X_LSTCore_realfiles_batch4 Jul 31, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Total removal of sdl/SDL #72

Total removal of sdl/SDL #72

VourMa commented Jul 30, 2024 •

edited

Loading

slava77 Jul 30, 2024

VourMa Jul 30, 2024

slava77 Jul 30, 2024

VourMa Jul 30, 2024

slava77 Jul 30, 2024

VourMa Jul 30, 2024

slava77 Jul 30, 2024

VourMa commented Jul 30, 2024

slava77 commented Jul 30, 2024

slava77 commented Jul 30, 2024

github-actions bot commented Jul 30, 2024

slava77 commented Jul 30, 2024

github-actions bot commented Jul 30, 2024

ariostas commented Jul 31, 2024

ariostas commented Jul 31, 2024

github-actions bot commented Jul 31, 2024

ariostas commented Jul 31, 2024

github-actions bot commented Jul 31, 2024

slava77 commented Jul 31, 2024

github-actions bot commented Jul 31, 2024

github-actions bot commented Jul 31, 2024

Total removal of sdl/SDL #72

Total removal of sdl/SDL #72

Conversation

VourMa commented Jul 30, 2024 • edited Loading

slava77 Jul 30, 2024

Choose a reason for hiding this comment

VourMa Jul 30, 2024

Choose a reason for hiding this comment

slava77 Jul 30, 2024

Choose a reason for hiding this comment

VourMa Jul 30, 2024

Choose a reason for hiding this comment

slava77 Jul 30, 2024

Choose a reason for hiding this comment

VourMa Jul 30, 2024

Choose a reason for hiding this comment

slava77 Jul 30, 2024

Choose a reason for hiding this comment

VourMa commented Jul 30, 2024

slava77 commented Jul 30, 2024

slava77 commented Jul 30, 2024

github-actions bot commented Jul 30, 2024

slava77 commented Jul 30, 2024

github-actions bot commented Jul 30, 2024

ariostas commented Jul 31, 2024

ariostas commented Jul 31, 2024

github-actions bot commented Jul 31, 2024

ariostas commented Jul 31, 2024

github-actions bot commented Jul 31, 2024

slava77 commented Jul 31, 2024

github-actions bot commented Jul 31, 2024

github-actions bot commented Jul 31, 2024

VourMa commented Jul 30, 2024 •

edited

Loading