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

ODD Strip Module Handling, main branch (2024.05.08.) #581

Merged
merged 4 commits into from
May 10, 2024

Conversation

krasznaa
Copy link
Member

@krasznaa krasznaa commented May 8, 2024

As discussed many times recently, currently we are handling all modules / surfaces as pixels when reconstructing ODD data. Which leads to some funky behaviour in its short and long strips.

As discussed on Mattermost, we can use a sort of hacky way to determine which modules / surfaces are 1D and 2D, by looking at the variations assigned to their 2 dimensions. Since the digitization config always describes 2 dimensions for the surfaces, even for strip modules. (It's a long story...)

With the info added to traccc::digitization_config and then to traccc::cell_module, I could make the host and device measurement creation algorithms take this information into account.

It did not cause any significant changes in our host<->device agreement numbers, those just moved around a tiny bit. But also, this did not seem to break anything either... 😉

krasznaa added 3 commits May 8, 2024 17:16
Based it on the RMS of the variation on the second dimension
of the binning data. Since this is set to a large value for strip
detectors in our current setup.
Filling it from the newly added peoprty in the digitization
data, during cell reading.
@krasznaa krasznaa added cuda Changes related to CUDA improvement Improve an existing feature sycl Changes related to SYCL cpu Changes related to CPU code labels May 8, 2024
@krasznaa krasznaa requested review from stephenswat and beomki-yeo May 8, 2024 15:40
Copy link
Member Author

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

Note that I also didn't touch the spacepoint creation algorithm in this PR. That one will need a bit more careful PR, since our device code currently assumes that it would create a fixed sized space point collection. Which would have to become a resizable one. 🤔

So, in a future PR...

@krasznaa
Copy link
Member Author

I'll open a PR with "truth maching code updates" still today. But until then, somebody should have a look at this PR. Since the truth matching code that I'll propose, absolutely relies on these changes.

Copy link
Contributor

@beomki-yeo beomki-yeo left a comment

Choose a reason for hiding this comment

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

I am OK with the change for the moment. I hope that it is not the permanent solution as you implied in the description above.

@krasznaa
Copy link
Member Author

This sort of "detector description" information will have to remain in some traccc specific format, so that we could give it to the measurement and spacepoint creation algorithms conveniently.

But the source of the information will not remain so hacky of course.

I've been working for a while on replacing our current cell_module type completely. It was just not the highest priority recently. But I'll have a proposal soon for how we should handle this info in our algorithms in the long term. 😉

@krasznaa krasznaa enabled auto-merge May 10, 2024 14:31
@krasznaa krasznaa merged commit 04241e8 into acts-project:main May 10, 2024
16 of 18 checks passed
@krasznaa krasznaa deleted the ODDStripModules-main-20240508 branch May 10, 2024 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cpu Changes related to CPU code cuda Changes related to CUDA improvement Improve an existing feature sycl Changes related to SYCL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants