Skip to content

refactor!: Packed strip space point calibration column#5424

Open
andiwand wants to merge 32 commits into
acts-project:mainfrom
andiwand:perf-seeding-precomputed-stripcolumns
Open

refactor!: Packed strip space point calibration column#5424
andiwand wants to merge 32 commits into
acts-project:mainfrom
andiwand:perf-seeding-precomputed-stripcolumns

Conversation

@andiwand

@andiwand andiwand commented May 9, 2026

Copy link
Copy Markdown
Contributor

@andiwand andiwand added this to the next milestone May 9, 2026
@github-actions

github-actions Bot commented May 9, 2026

Copy link
Copy Markdown
Contributor

📊: Physics performance monitoring for e458cf3

Full contents

physmon summary

@github-actions github-actions Bot added Component - Examples Affects the Examples module Track Finding labels May 9, 2026
@andiwand

andiwand commented May 9, 2026

Copy link
Copy Markdown
Contributor Author

Elevates the pre-computable strip space point information out of the seeding code. This way the computation only needs to happen once, but the user is now responsible for the computation. Therefore this is a breaking change.

Also allows to deduplicate the stripCoordinateCheck again. In a followup PR I plan to elevate this this function and rename it to something like stripSpacePointCalibration to make it useable in the parameter estimation.

To be seen what performance difference this makes for ITk strip seeding

After fighting the performance I get a marginal 2% improvement in Athena with the SPOT script. I am back to two functions again but elevated them now to public functions in Core/SpacePointerFormation2/StripSpacePointBuilder.hpp.

With the single function and the 5 calibration vectors attached to the space point I saw about ~10% slowdown. My interpretation is that all of these are in the cache and loading them from there is slower than recalculating them from 4 calibration vectors. I think this will highly depend on the CPU and scenario but lets not go down that road.

I also experimented with using Eigen to various degrees but the more we use it the more it slows down...

What is left from this is mainly a refactor and some breaking renames that I believe overall improve things. Next I would explore a bit how to best estimate the seed params using the calibration.

@andiwand andiwand left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

A few things I was not sure about while trying to understand the code

Comment thread Core/include/Acts/SpacePointFormation2/StripSpacePointBuilder.hpp Outdated
Comment thread Core/include/Acts/SpacePointFormation2/StripSpacePointBuilder.hpp Outdated
Comment thread Core/include/Acts/SpacePointFormation2/StripSpacePointBuilder.hpp Outdated
Comment thread Core/include/Acts/EventData/StripSpacePointCalibrationDetails.hpp Outdated
Comment thread Core/include/Acts/EventData/StripSpacePointCalibrationDetails.hpp Outdated
Comment thread Core/include/Acts/SpacePointFormation2/StripSpacePointBuilder.hpp Outdated
Comment thread Core/include/Acts/SpacePointFormation2/StripSpacePointBuilder.hpp Outdated
Comment thread Core/include/Acts/SpacePointFormation2/StripSpacePointBuilder.hpp Outdated
@andiwand andiwand changed the title perf!: Precomputed strip columns for seeding refactor!: Packed strip space point calibration columns May 9, 2026
@andiwand andiwand changed the title refactor!: Packed strip space point calibration columns refactor!: Packed strip space point calibration column May 10, 2026
@mvessell96

Copy link
Copy Markdown
Contributor

Elevates the pre-computable strip space point information out of the seeding code. This way the computation only needs to happen once, but the user is now responsible for the computation. Therefore this is a breaking change.
Also allows to deduplicate the stripCoordinateCheck again. In a followup PR I plan to elevate this this function and rename it to something like stripSpacePointCalibration to make it useable in the parameter estimation.
To be seen what performance difference this makes for ITk strip seeding

After fighting the performance I get a marginal 2% improvement in Athena with the SPOT script. I am back to two functions again but elevated them now to public functions in Core/SpacePointerFormation2/StripSpacePointBuilder.hpp.

With the single function and the 5 calibration vectors attached to the space point I saw about ~10% slowdown. My interpretation is that all of these are in the cache and loading them from there is slower than recalculating them from 4 calibration vectors. I think this will highly depend on the CPU and scenario but lets not go down that road.

I also experimented with using Eigen to various degrees but the more we use it the more it slows down...

What is left from this is mainly a refactor and some breaking renames that I believe overall improve things. Next I would explore a bit how to best estimate the seed params using the calibration.

Yes I tried something similar (ok but much less elegant) originally and also came to the conclusion that loading from cache was slower than recalculating them but also was not sure how much this result could be trusted for the same CPU/scenario reason so left it unmentioned ;) This is ultimately why I settled on the messy two function solution which I wasn't exactly happy with but was the fastest implementation out of the stuff I threw at the wall

@andiwand

Copy link
Copy Markdown
Contributor Author

I also tried using only the function with the pre-calculated struct and it seems to produce the same timing. Maybe we can deduplicate to this variant? Or did it behave differently for you?

There are a few things I was not sure about and left a few comments above, maybe you have an opinion on these @mvessell96.

If we could reduce the information necessary for the calibration it might go faster but I didn't have a great idea about it.

@mvessell96

mvessell96 commented May 10, 2026

Copy link
Copy Markdown
Contributor

I also tried using only the function with the pre-calculated struct and it seems to produce the same timing. Maybe we can deduplicate to this variant? Or did it behave differently for you?

There are a few things I was not sure about and left a few comments above, maybe you have an opinion on these @mvessell96.

If we could reduce the information necessary for the calibration it might go faster but I didn't have a great idea about it.

I think it gave like O(5-8%) which was a much bigger number when I put this in, I think likely some of the other recent improvements have eaten most of the gain? Since we are likely getting to the bad top sps less often

@andiwand

Copy link
Copy Markdown
Contributor Author

Ok good to know! Then lets just keep it - doesn't hurt

@andiwand

Copy link
Copy Markdown
Contributor Author

I coded up a little toy visualization and the current implementation looks indeed correct https://github.com/andiwand/cern-scripts/blob/main/scripts/2026-05-10_strip-space-point-calibration.py

image

@sonarqubecloud

Copy link
Copy Markdown

@andiwand andiwand modified the milestones: next, v47.0.0 May 22, 2026
@andiwand andiwand marked this pull request as ready for review June 11, 2026 18:14
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants