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

Host Clusterization Reorganization, main branch (2024.04.16.) #543

Merged

Conversation

krasznaa
Copy link
Member

As I have been advertising for a few days already, in order to progress with making the full ODD reconstruction chain working with CUDA, the clusterization code needs to be revised a bit. This is a first step for that.

Since I'll end up touching a lot of files, I decided that I should just modify things with the core/host clusterization algorithms at first. The device code (which is actually the more important for us right now...) will come in a follow-up PR.

The code does a few things:

  • Renamed all of the algorithms;
    • Moved all of them into the traccc::host namespace, and added the _algorithm postfix to all of their names for consistency;
    • Renamed component_connection to sparse_ccl_algorithm in the end, in an effort to make it a bit clearer through the naming what type of clusterization is used by the different algorithms. (The device code will be updated accordingly where needed.)
  • Made all the algorithms take (const) views as their inputs, instead of host objects. This should make it a little easier to combine host and device algorithms into a single chain in the future, if needed.
  • Removed the templating from the implementation functions, and moved all of them under core/include/traccc/clusterization/details, and into the traccc::details namespace. (With the implementation files going in the core/include/traccc/clusterization/impl subdirectory.
    • I hope to make more use of these functions in the device functions later on, but didn't actually test that for this PR just yet.

Since some of the changes may look suspicious, I checked what these changes do to our existing throughput measurements. (Not a thorough test, just a simple one.) The code from the PR does:

[bash][atspot01]:traccc > ./traccc/out/build/sycl/bin/traccc_throughput_st

Running Single-threaded host-only throughput tests

>>> Detector Options <<<
  Detector file       : tml_detector/trackml-detector.csv
  Material file       : 
  Surface rid file    : 
  Use detray::detector: no
  Digitization file   : tml_detector/default-geometric-config-generic.json
>>> Input Data Options <<<
  Input data format             : csv
  Input directory               : tml_full/ttbar_mu20/
  Number of input events        : 1
  Number of input events to skip: 0
>>> Clusterization Options <<<
  Target cells per partition: 1024
>>> Track Seeding Options <<<
  None
>>> Throughput Measurement Options <<<
  Cold run event(s) : 10
  Processed event(s): 100
  Log file          : 

Reconstructed track parameters: 234000
Time totals:
                  File reading  240 ms
            Warm-up processing  118 ms
              Event processing  1188 ms
Throughput:
            Warm-up processing  11.8719 ms/event, 84.2324 events/s
              Event processing  11.8869 ms/event, 84.1263 events/s
[bash][atspot01]:traccc >

While with the current main branch I see:

[bash][atspot01]:traccc > ./traccc/out/build/sycl/bin/traccc_throughput_st

Running Single-threaded host-only throughput tests

>>> Detector Options <<<
  Detector file       : tml_detector/trackml-detector.csv
  Material file       : 
  Surface rid file    : 
  Use detray::detector: no
  Digitization file   : tml_detector/default-geometric-config-generic.json
>>> Input Data Options <<<
  Input data format             : csv
  Input directory               : tml_full/ttbar_mu20/
  Number of input events        : 1
  Number of input events to skip: 0
>>> Clusterization Options <<<
  Target cells per partition: 1024
>>> Track Seeding Options <<<
  None
>>> Throughput Measurement Options <<<
  Cold run event(s) : 10
  Processed event(s): 100
  Log file          : 

Reconstructed track parameters: 234000
Time totals:
                  File reading  251 ms
            Warm-up processing  123 ms
              Event processing  1236 ms
Throughput:
            Warm-up processing  12.3891 ms/event, 80.7159 events/s
              Event processing  12.3669 ms/event, 80.8612 events/s
[bash][atspot01]:traccc >

So if anything, these changes just improve our code's performance. (But don't read too much into the results. I just ran this small statistics test a few times, just to make sure that obvious warm-up effects would not play a role.)

@krasznaa krasznaa added refactor Change the structure of the code cpu Changes related to CPU code examples Changes to the examples labels Apr 16, 2024
Renamed the class, and made sure that it would work with
"device types" instead of using templating unnecessarily.
Renamed the class, and made sure that it would work with
"device types" instead of using templating unnecessarily.
Renamed the class, and made sure that it would work with
"device types". While extracting its core calculation into
a helper function that the device code could reuse.
Moved all of the clusterization (adjacent) algorithms into the
traccc::host namespace, and renamed traccc::component_connection_algorithm
to traccc::host::sparse_ccl_algorithm.
@krasznaa krasznaa force-pushed the ClusterizationReorg-main-20240415 branch from e4fdd25 to 4c43e1a Compare April 16, 2024 09:34
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.

Looks good to me.
Just note that we might need to move spacepoint_formation to seeding directory at some point because spacepoints are only used for the seeding algorithm

@krasznaa
Copy link
Member Author

Yepp, I fully agree. I thought about possibly doing that in this MR, but then decided against it.

Let's do that along with getting rid of https://github.com/acts-project/traccc/blob/main/core/include/traccc/seeding/experimental/spacepoint_formation.hpp, and making the spacepoint formation use Detray. (Or a version of Detray, to be seen. 🤔 I have some ideas...) In a PR in the non-too-distant future. 😉

@krasznaa krasznaa merged commit 4854c65 into acts-project:main Apr 16, 2024
17 of 18 checks passed
@krasznaa krasznaa deleted the ClusterizationReorg-main-20240415 branch April 16, 2024 18:13
@beomki-yeo
Copy link
Contributor

I don't know what would be your idea -- but please note that just using transform3 matrix won't be enough. transform3-based coordinate transform only works for 2d cartesian measurement.

But it will never work on the straw tube (still cartesian but xyz order is swapped) and annulus shape with polar coordinate

@beomki-yeo
Copy link
Contributor

Thoguh, It is also true that we might not be able to make any spacepoints from straw or 1d strip

@niermann999
Copy link
Contributor

However, we will need to observe alignment conditions data, right?

@stephenswat
Copy link
Member

stephenswat commented Apr 17, 2024

annulus shape with polar coordinate

Out of curiosity, is there anything preventing us from composing a Cartesian-to-polar transformation and a homogeneous Cartesian transformation to make this work in exactly the same way that it does for 2D rectangular surfaces?

Edit: never mind, makes sense now that I think of it.

@beomki-yeo
Copy link
Contributor

However, we will need to observe alignment conditions data, right?

Right. Anyway we don't have to play a trick on spacepoint formation beyond what is in the experimental version. Moving away from the detector object will just make it less robust

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 examples Changes to the examples refactor Change the structure of the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants