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

Introduce traccc::silicon_detector_description, main branch (2024.06.21.) #627

Merged

Conversation

krasznaa
Copy link
Member

@krasznaa krasznaa commented Jun 21, 2024

This is going to be a big, and painful PR... It attempts to completely re-design how we deal with "detector description information" in the algorithms.

At the moment we create two "event data" collections as input to our algorithm chain.

  • traccc::cell_collection_types: Describing the actual cell (pixel/strip) data;
  • traccc::cell_module_collection_types: Describing the modules that the cells are on.

The latter collection we create separately for each event, and have to ship event-by-event to a device to use.

Instead of doing that, one should (of course...) only copy the description of every detector module to the device once (per IoV...). And have the cells refer to that global object. The detray::detector object is not enough for this as it happens. Since the tracking geometry doesn't (and shouldn't) know about the segmentation of the modules, the signal thresholds in them, etc. This is where the traccc::silicon_detector_description type comes in. 😄

It's an SoA container based on vecmem::edm. Ensuring that since most clients will only need to access a small part of a module's info at a time, this would be done efficiently.

At the same time I also created some "named Detray detector types" in the code. With the intention of bringing the templating of the track finding and fitting algorithms under control. And also allowing to centralise the I/O of the geometry and detector description info a little better. Note that with all these "geometry updates" the client code for constructing traccc::default_detector::host and traccc::silicon_detector_description::host becomes a bit less sketchy than what we're doing now. (Most importantly, we don't need to pass around a weird std::map between function calls in this setup.)

Since traccc::cell_module is used in half of our source files (with a bit of exaggeration), I didn't yet manage to update all of the project to the new status quo yet. Hence the draft status for now. I just wanted people to have a look already, since I've been working on this code for long enough already... (Check the name of my branch...)

@krasznaa krasznaa added feature New feature or request tests Make sure the code keeps working cuda Changes related to CUDA sycl Changes related to SYCL cpu Changes related to CPU code edm Changes to the data model kokkos Changes related to Kokkos alpaka Changes related to Alpaka examples Changes to the examples labels Jun 21, 2024
@beomki-yeo
Copy link
Contributor

I might be OK with that but this is also time we need to think of the extension to wire measurements.
As we elaborate detector description for pixel/strip based on the digitization it is going to be difficult to harmonize with gaseous detector measurement such as straw tube or drift chamber. They are not described by cell type and don't have segmentation like pixel does.

This may have nothing to do with this PR but it is not bad to keep that in mind in the detector description design.

@krasznaa
Copy link
Member Author

I don't even know conceptually what we'll want to do with clusterization and seeding in gas detectors. 🤔 But we'll need to find a unified interface for their "detector description" eventually for sure...

@stephenswat
Copy link
Member

Does anyone really do seeding in gas detectors? Correct me if I am wrong, but I don't think ATLAS does, right?

@beomki-yeo
Copy link
Contributor

beomki-yeo commented Jun 21, 2024

For wire measurements, it is more like a peak search in the ADC data rather than clusterization
Still I am not also sure about exact data type. We might need to ask Phase 1 reconstruction experts.

Does anyone really do seeding in gas detectors?

Maybe not for ATLAS TRT. But other experiments, yes

@krasznaa
Copy link
Member Author

Yes, we'll need to discuss about this soon. Since in general the input EDM of the relevant algorithms will probably need to be generalized in the end. 🤔 Eventually we'll probably want to name our current cell collection to something like Acts::Acc::SiCellCollection. (Just to foreshadow the general renaming that awaits us during the migration into the Acts repository...)

@krasznaa krasznaa force-pushed the DetectorReadingUpdate-main-20240425 branch 2 times, most recently from fe71f43 to bfebfa4 Compare June 22, 2024 19:06
Copy link
Member

@stephenswat stephenswat left a comment

Choose a reason for hiding this comment

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

Overall I really like the look of this; I appreciate the simplification of having the same type for all of this information across the algorithms.

I'll give it a deeper look on Monday.

@krasznaa krasznaa force-pushed the DetectorReadingUpdate-main-20240425 branch from bfebfa4 to cf62d37 Compare June 25, 2024 09:44
@beomki-yeo
Copy link
Contributor

beomki-yeo commented Jul 30, 2024

This will be helpful if @krasznaa is going to consider the detector description for non-Si-based detectors
https://gitlab.cern.ch/atlas/athena/-/blob/main/InnerDetector/InDetCalibTools/TRT_CalibTools/src/Calibrator.h#L77

@krasznaa krasznaa force-pushed the DetectorReadingUpdate-main-20240425 branch from 2f4f51e to cc3cd3a Compare July 31, 2024 14:29
@krasznaa krasznaa force-pushed the DetectorReadingUpdate-main-20240425 branch 2 times, most recently from 63a27b6 to fb6b1fb Compare August 13, 2024 19:03
@krasznaa krasznaa force-pushed the DetectorReadingUpdate-main-20240425 branch from fb6b1fb to 646b0ee Compare August 15, 2024 16:03
@krasznaa krasznaa force-pushed the DetectorReadingUpdate-main-20240425 branch from 94dad51 to eda4e85 Compare August 22, 2024 14:35
The traccc algorithms will need to work on a concrete list of detector
types, they cannot be fully templated.

Started with adding I/O for the types that support it.
Declared an SoA container called traccc::detector_description that
would hold all of the detector description data needed by the
measurement and spacepoint creation algorithms. Fillable both from
the old-style CSV files used for the TrackML detector, and from
JSON Detray geometry configuration files.
Updated the host-based measurement creation algorithm
to use the new type.
In a quite broken state...
While fixing the measurement creation logic for 1D modules,
and fixing the code formatting of the recently modified files.
…ption.

At the same time harmonized the CUDA and SYCL code implementation
a little.
At the same time fixing some mistakes that I added while updating
the CUDA clusterization code for using traccc::detector_description.
Also ran some minimal tests with them, but nothing super
thorough.
With practically no runtime testing for now.
At the same time updated the code to use the same command line
setup as all the other executables do.

Removed the other OpenMP example, as it was not actually doing
anything useful anyway...
At the same time simplified the benchmarking code
a little.
One failure was because of a difference in the "module link"
variable of cells. Since in the new scheme this link would
mean an index in the "global" DD object.

The other failure was however because of a genuine bug
in the spacepoint reading that I introduced while updating
the I/O code to use the new DD type.
In preparation for introducing detector description objects for
other types of detectors as well later on.
Following Joana's recommendation.
@krasznaa krasznaa force-pushed the DetectorReadingUpdate-main-20240425 branch from eda4e85 to e379d16 Compare September 11, 2024 13:15
Removed the variance_z variable, as it was not used anymore.
Named the "geometry identifier" variables as geometry_id()
and acts_geometry_id(), following Beomki's suggestion.
The code no longer uses branching to set the 2D coordinates
differently for 1D and 2D measurements. The second coordinate
is just assumed to be ignored for 1D measurements instead.
@krasznaa
Copy link
Member Author

Over the weekend I looked at making the spacepoint formation use detray::detector instead of the placement() variable of traccc::silicon_detector_description. That part of the change is actually super easy, our algorithmic code uses placement() in exactly a single place at the moment.

But I had to realize that we have a gazillion tests that use TML data at the moment. We'll need to change all of those tests to use ODD data once we go for retiring TML compatibility completely. Which is definitely something that we need to do soon. But I decided to not do it in this PR after all.

I'd propose to keep traccc::silicon_detector_description::placement() in this PR, as is. Then I'll / we'll remove all TML support in a follow-up PR. We'll anyway need to check carefully how that will interact with the changes that @beomki-yeo has been working on recently. (Since he's also been working on removing TML support.)

Everyone, please give this another look. It would be great to push it through the finish line finally.

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 approve this PR gladly although there are a couple of issues that need to be handled in the near furure (e.g. strip in spacepoint formation, copy of transform objects)

@krasznaa krasznaa merged commit 4370163 into acts-project:main Sep 16, 2024
18 of 24 checks passed
@krasznaa krasznaa deleted the DetectorReadingUpdate-main-20240425 branch October 30, 2024 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
alpaka Changes related to Alpaka cpu Changes related to CPU code cuda Changes related to CUDA edm Changes to the data model examples Changes to the examples feature New feature or request kokkos Changes related to Kokkos sycl Changes related to SYCL tests Make sure the code keeps working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants