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

feat!: Resurrect original AdaptiveGridTrackDensity; Move new impl to SparseGridTrackDensity #3340

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Jul 1, 2024

This is an attempt to resurrect the original AdaptiveGridTrackDensity to compare it's performance against the new SparseGridTrackDensity.

The main reason for this is that the algorithms diverged significantly and it would be good to capture this by different names. This also allows for CPU and physics performance comparisons which is currently looked at.

blocked by

@github-actions github-actions bot added Component - Core Affects the Core module Infrastructure Changes to build tools, continous integration, ... Component - Examples Affects the Examples module Vertexing labels Jul 1, 2024
@andiwand
Copy link
Contributor Author

andiwand commented Jul 2, 2024

This is a bit more complicated than I anticipated. There have been a serious of changes to the AdaptiveGridTrackDensity, some of them we want to keep and some of them we want to revert.

I think this has to be done manually since this also interfered with large scale vertexing refactor. Overall we should keep the improved comments and interfaces while the algorithm should stay as it was with its data structures.

@paulgessinger
Copy link
Member

You don't think we can keep #2745 @andiwand?

@andiwand
Copy link
Contributor Author

andiwand commented Jul 2, 2024

You don't think we can keep #2745 @andiwand?

I would definitely keep this in SparseGridTrackDensity which is the current implementation of AdaptiveGridTrackDensity. Looking at the old implementation of AdaptiveGridTrackDensity it seems to be that the constant size has performance benefits. If there is an alternative I would also like to drop the template param.

@paulgessinger
Copy link
Member

paulgessinger commented Jul 2, 2024

Did we not measure this back then? I'd be surprised if that's what makes a difference in performance here.

If it does after all, I'd want to attempt to use the compile time variables by dispatching in a compilation unit.

@andiwand
Copy link
Contributor Author

andiwand commented Jul 2, 2024

I don't think we did a full comparison of the std::unordered_map vs the std::vector implementation. Part of this is that track density objects can live on the stack with known size.

@andiwand andiwand changed the title feat!: Resurrect original AdaptiveGridTrackDensity vs SparseGridTrackDensity feat!: Resurrect original AdaptiveGridTrackDensity; Move new impl to SparseGridTrackDensity Jul 3, 2024
@andiwand andiwand added this to the next milestone Jul 4, 2024
@andiwand andiwand marked this pull request as ready for review July 6, 2024 06:35
@andiwand andiwand added the 🛑 blocked This item is blocked by another item label Jul 6, 2024
@andiwand
Copy link
Contributor Author

andiwand commented Jul 6, 2024

This is the performance I measured for CPU and physics. Looks surprisingly comparable to me.

image

I am not sure if we want to proceed with porting the original AdaptiveGridTrackDensity now. One reason to do so is that the algorithm quite diverged and it might be confusing to keep the name for the newer version. Also the newer one was extended to time while the old one was not adoptable in this direction. On the other hand the principle is quite the same while we use a different data structure as a backend.

@andiwand andiwand removed the 🛑 blocked This item is blocked by another item label Jul 22, 2024
@andiwand
Copy link
Contributor Author

dropping this for now as there are no apparent gains in merging this

@andiwand andiwand closed this Jul 22, 2024
Copy link

sonarcloud bot commented Jul 22, 2024

@paulgessinger paulgessinger modified the milestones: next, v36.1.0 Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Core Affects the Core module Component - Examples Affects the Examples module Infrastructure Changes to build tools, continous integration, ... Vertexing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants