-
Notifications
You must be signed in to change notification settings - Fork 52
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
Refactor the spacepoint formation #719
Refactor the spacepoint formation #719
Conversation
8e8b114
to
086287b
Compare
086287b
to
7d787af
Compare
std::make_tuple("tml_detector/trackml-detector.csv", | ||
"tml_detector/default-geometric-config-generic.json", | ||
"tml_full/single_muon/", 9))); | ||
std::make_tuple("geometries/odd/odd-detray_geometry_detray.json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the spacepoint formation always require the detray geometry, the test files are changed to ODD from TML. The test compares the position of the found spacepoints (spacepoint_formation_algorithm
) and truth spacepoints (hits.csv
). Even though it is expected that there must be some deviations caused by the measurement_creation
, the test requires about 20% uncertainties which is pretty bad. This probably explains the bad efficiency we have observed in the past - This needs to be followed up in the later PRs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not the greatest that we still have a lot of "if Detray detector then ..." checks in the code, but I think you're right. Let's be pragmatic, and just make the code work for now. It will indeed be better to get rid of the TML setup in follow-up steps.
Generally I'm happy with how you went about this, just had a couple of comments on a quick look. 🤔
device/common/include/traccc/seeding/device/impl/form_spacepoints.ipp
Outdated
Show resolved
Hide resolved
eba9cd3
to
d3f35a2
Compare
The performance remains the same Command
Main
PR
|
d3f35a2
to
f7908ea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, let's go like this for now. As I'm on board with the overall direction.
PR acts-project#719 refactored the spacepoint formation to use the detray detector. While this was and remains a good idea, the introduction of the detray detector massively increases the register use of the kernel, to the point that I was seeing "too many resources requested for launch" errors. This commit modifies the kernel to specify appropriate launch bounds, allowing the compiler to better tune the register usage and resolve the bug.
PR acts-project#719 refactored the spacepoint formation to use the detray detector. While this was and remains a good idea, the introduction of the detray detector massively increases the register use of the kernel, to the point that I was seeing "too many resources requested for launch" errors. This commit modifies the kernel to specify appropriate launch bounds, allowing the compiler to better tune the register usage and resolve the bug.
PR acts-project#719 refactored the spacepoint formation to use the detray detector. While this was and remains a good idea, the introduction of the detray detector massively increases the register use of the kernel, to the point that I was seeing "too many resources requested for launch" errors. This commit modifies the kernel to specify appropriate launch bounds, allowing the compiler to better tune the register usage and resolve the bug.
PR acts-project#719 refactored the spacepoint formation to use the detray detector. While this was and remains a good idea, the introduction of the detray detector massively increases the register use of the kernel, to the point that I was seeing "too many resources requested for launch" errors. This commit modifies the kernel to specify appropriate launch bounds, allowing the compiler to better tune the register usage and resolve the bug.
This are the changes that I have been suggesting for long time.
spacepoint_formation
takes detray detector instead of detector description. Direct use ofpoint_to_global
withtransform_matrix
should be avoided. Most importantly, the local-to-global transformation of annulus shape (Polar coordinate) does not work withpoint_to_global
which only works for Cartesian coordinate.As detector description is not required (and should not be used), I moved its directory to
seeding
where theexperimental::speacepoing_formation
is located. This makes sense because spacepoints are technically only used for seeding.+ Spacepoint formation is only applied to the 2D measurements as we do not support 1D measurements sp formation. Supporting 1D measurement will need to be followed up by further studiesLet me have 2D condition in the next PR (to test the performance without changing the actual results...)