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

Command Line Options Cleanup, main branch (2024.03.05.) #525

Merged
merged 2 commits into from
Mar 5, 2024

Conversation

krasznaa
Copy link
Member

@krasznaa krasznaa commented Mar 5, 2024

This all started out from a report on Mattermost that the throughput measuring executables are not working at the moment. But instead of merely fixing a typo in the "options code", I decided that it was time to do a thorough cleanup of it.

I did a number of things:

  • Got rid of all templating from the "options structs". I do not believe that we would have any need for using either float or double precision when specifying runtime parameters on the command line. So these options are always just specified as float-s, and then those configuration parameters may be used in double precision calculations later on.
  • Changed the name of some of the structs. Weirdly enough the names of the source files were in all cases how I wanted to call the structs themselves. So this was a pretty unfortunate inconsistency in the code so far. 😦
  • Switched all of the structs to a formalism where they would make boost::program_options write the configured parameters into C++ variables "directly". Instead of declaring the option first, and then extracting the option ourselves later on.
    • I still kept all of the read(...) functions, since in a few cases we still need to do some non-trivial operations while interpreting the user-provided arguments. Though much of the code from these read(...) functions could be removed.
    • This is mainly to reduce code repetition. Which was the reason for the typo in the first place. 🤔
  • Introduced "output operators" for all of the options structs. Similar to what we had already for a few of them.
    • Then updated all of the example applications to print their configured parameters using these helper operators. So that they would present a slightly more uniform look-and-feel. Like:
[bash][atspot01]:traccc > ./out/build/sycl/bin/traccc_seq_example_sycl --detector-file=tml_detector/trackml-detector.csv --digitization-config-file=tml_detector/default-geometric-config-generic.json --input-directory=tml_full/ttbar_mu20/ --events=2

Running the full tracking chain using SYCL

>>> Common options <<<
  Input data format            : csv
  Input directory              : tml_full/ttbar_mu20/
  Events                       : 2
  Skip                         : 0
  Target cells per partition   : 1024
  Check performance            : 0
  Perform ambiguity resolution : 1
>>> Detector options <<<
  Detector file : tml_detector/trackml-detector.csv
  Material file : 
  Grid file     : 
>>> Full tracking chain options <<<
  Digitization configuration file: tml_detector/default-geometric-config-generic.json

Running Seeding on device: NVIDIA GeForce RTX 2060
==> Statistics ... 
- read    95678 cells from 12529 modules
- created (cpu)  0 measurements
- created (cpu)  0 spacepoints
- created (sycl) 27862 spacepoints     
- created  (cpu) 0 seeds
- created (sycl) 3924 seeds
==>Elapsed times...
           File reading  (cpu)  228 ms
         Clusterization (sycl)  4 ms
                Seeding (sycl)  4 ms
           Track params (sycl)  0 ms
                     Wall time  238 ms
[bash][atspot01]:traccc >

This whole cleanup, I believe, was in the end very useful. Since I now have a new idea for why track finding in #509 would not be working. 🤔

@krasznaa krasznaa added bug Something isn't working cleanup Makes the code all clean and tidy examples Changes to the examples labels Mar 5, 2024
@krasznaa krasznaa requested a review from beomki-yeo March 5, 2024 14:29
@beomki-yeo
Copy link
Contributor

In general I agree with all points so will hit approval once it passes the CI

@krasznaa krasznaa force-pushed the OptionsCleanup-main-20240305 branch from 6c87150 to 2bb95b8 Compare March 5, 2024 15:06
@krasznaa
Copy link
Member Author

krasznaa commented Mar 5, 2024

Note that I also removed all of the "required options" as well. 🤔 Providing default values for absolutely all of the options. Because I've always found it pretty painful that none of our executables can just be run as-is. 😦 Well, now they can. At least most of them... (Since for some of the options it's not easy to find universal defaults... 😦)

So, something like this:

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

Running the full tracking chain on the host

>>> Common options <<<
  Input data format            : csv
  Input directory              : tml_full/ttbar_mu20/
  Events                       : 1
  Skipped events               : 0
  Target cells per partition   : 1024
  Check performance            : 0
  Perform ambiguity resolution : 1
>>> Detector options <<<
  Detector file : tml_detector/trackml-detector.csv
  Material file : 
  Grid file     : 
>>> Full tracking chain options <<<
  Digitization configuration file: tml_detector/default-geometric-config-generic.json

==> Statistics ... 
- read    54988 cells from 6997 modules
- created 15919 measurements. 
- created 15919 space points. 
- created 2340 seeds
[bash][atspot01]:traccc > ./out/build/sycl/bin/traccc_seq_example_cuda 

Running the full tracking chain using CUDA

>>> Common options <<<
  Input data format            : csv
  Input directory              : tml_full/ttbar_mu20/
  Events                       : 1
  Skipped events               : 0
  Target cells per partition   : 1024
  Check performance            : 0
  Perform ambiguity resolution : 1
>>> Detector options <<<
  Detector file : tml_detector/trackml-detector.csv
  Material file : 
  Grid file     : 
>>> Full tracking chain options <<<
  Digitization configuration file: tml_detector/default-geometric-config-generic.json

==> Statistics ... 
- read    54988 cells from 6997 modules
- created (cpu)  0 measurements     
- created (cpu)  0 spacepoints     
- created (cuda) 15919 spacepoints     
- created  (cpu) 0 seeds
- created (cuda) 2340 seeds
==>Elapsed times...
           File reading  (cpu)  136 ms
         Clusterization (cuda)  59 ms
                Seeding (cuda)  4 ms
           Track params (cuda)  2 ms
                     Wall time  203 ms
[bash][atspot01]:traccc >

@krasznaa
Copy link
Member Author

krasznaa commented Mar 5, 2024

It's ready now, after having updated the Alpaka and Kokkos executables as well. 😉

@niermann999 niermann999 self-requested a review March 5, 2024 17:34
krasznaa added 2 commits March 5, 2024 18:36
Made all structs behave the same way, and introduced print operators
for all of them.
Made all of the applications print their configuration parameters
in a uniform way, using the helper code introduced with the
"options code".
@krasznaa krasznaa force-pushed the OptionsCleanup-main-20240305 branch from cd636d8 to 93dc0d1 Compare March 5, 2024 17:36
@krasznaa krasznaa merged commit 3be655e into acts-project:main Mar 5, 2024
18 checks passed
@krasznaa krasznaa deleted the OptionsCleanup-main-20240305 branch March 5, 2024 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cleanup Makes the code all clean and tidy examples Changes to the examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants