-
Notifications
You must be signed in to change notification settings - Fork 13
Navigation performance benchmark - implementing #49 #530
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
base: main
Are you sure you want to change the base?
Navigation performance benchmark - implementing #49 #530
Conversation
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.
Pull Request Overview
This PR adds a new benchmark generator (RMGGeneratorBenchmark) for measuring geometry navigation performance in Geant4 simulations. The generator systematically samples particles across three orthogonal planes (XZ, YZ, XY) using a grid pattern and records timing data to identify potential geometry bottlenecks.
Key Changes:
- Implements
RMGGeneratorBenchmarkclass that shoots geantinos through the detector geometry in a systematic grid pattern - Adds
RMGBenchmarkOutputSchemeto store benchmark timing data in three separate ntuples (one per plane) - Integrates the benchmark generator into the existing generator framework with CLI configuration options
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
src/RMGMasterGenerator.cc |
Adds benchmark generator to the generator factory switch statement |
src/RMGGeneratorBenchmark.cc |
Implements the benchmark generator with grid-based particle sampling and timing measurement |
src/RMGBenchmarkOutputScheme.cc |
Implements output scheme for storing benchmark data to three auxiliary ntuples |
src/CMakeLists.txt |
Registers new source and header files in build system |
include/RMGUserInit.hh |
Registers benchmark output scheme as an optional output scheme |
include/RMGMasterGenerator.hh |
Adds kBenchmark enum value to Generator enumeration |
include/RMGGeneratorBenchmark.hh |
Header file declaring the benchmark generator class and its members |
include/RMGBenchmarkOutputScheme.hh |
Header file declaring the benchmark output scheme class |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gipert
left a comment
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.
Thanks Mo, I had a quick look and left some first comments.
I think just naming things with "benchmark" is not enough, we should also say "geometry". I would use everywhere either GeometryBenchmark or GeoBenchmark (if you prefer shorter names)
Also, the 3D thing is quite some code and I'm not convinced that it's useful... maybe this can come in a future PR
I was thinking about names. We could maybe use an even shorter name like |
yes I agree. I care more about consistency than the exact name. |
|
Implemented the suggestions consisting of:
In addition to that, there was also an internal calculation change:
I will now focus on implementing the gdml generator as outlined above. |
|
I had to approve the workflow, apparently giving you "triage" permissions does not mean you are "trusted". We should look into this, it's a bit annoying. |
|
No, I think this is because the last commit was by pre-commit and that is not a user with a repository role?
|
|
Ah ok, good then. Maybe we can also give it triage permissions? Not sure it's possible |
|
Added a |
|
I will add some documentation while waiting for further suggestions. |
|
I guess this PR should not yet include an overlap identifier. Though since |
|
Added terminal histogram output with Also, changed so that if there is already a box world volume, that one is enlarged instead of nesting previous world volumes inside one another. |
|
Could you rewrite the git history, tidy up and remove pre-commit bot commits? If you let the bot push them, the CI needs approval to run... |
|
very ugly... if there is no horizontal option (like in Julia's BenchmarkTools) i would skip this |
|
Not with this package AFAIK histoprint |
|
This package can generate sparklines: deeplook/sparklines |
|
watch out when you add dependencies: they need to be on conda-forge and pypi |
|
anyways now that i think about it i would not add another dependency just for a fancy plot... |
|
FYI this is what would be possible with the above package: If you are not interested, no worries. |
style: pre-commit fixes style: pre-commit fixes Update include/RMGGeneratorBenchmark.hh Co-authored-by: Copilot <[email protected]> Update include/RMGGeneratorBenchmark.hh Co-authored-by: Copilot <[email protected]> Implementing suggestions
…n calculation time Update python/remage/geometry_benchmark/cli.py Co-authored-by: Manuel Huber <[email protected]>
Introduces logic to extract and benchmark individual logical volumes from complex GDML geometries, including new CLI options for selecting a component and specifying buffer space around the geometry. Refactors geometry handling to wrap extracted components in a temporary, buffered world volume for isolated benchmarking. Improves summary generation to reflect component-specific output naming and corrects extent calculation for visualization. Enhances sampling region logging for clarity on simulation setup. style: pre-commit fixes Incorporating Luigis suggestions style: pre-commit fixes
The text is ai generated, but I checked it and through out stuff when it was yapping. style: pre-commit fixes fix docstring? style: pre-commit fixes Improves output naming and adds simulation time histograms Standardizes output file naming by introducing an explicit file stem variable, ensuring more predictable and descriptive output filenames, especially when extracting logical volumes. Refactors related CLI and summary generation logic to use this file stem throughout. Expands world volume resizing to avoid unnecessary registry duplication and ensure correct world volume updates. Adds a simulation time histogram printout using histoprint for better performance diagnostics. Includes minor doc and formatting fixes for clarity. style: pre-commit fixes style: pre-commit fixes precommit skip
…tions fixed some stuff
…ritzNeuberger/remage into navigaion_benchmark_utilities
|
I had a quick look, i think there is a lot of unnecessary logic (this is typical of the AI). could you review and try to simplify? even if it works, we should try to keep this code maintainable |
- Refactor geombench CLI and GDML handling, removing unnecessary complexity in util functions, removed macro file generation, piping it directly to the cli - Changed Out to OutFormat where applicable - Left try-except logic in to delte the tmp gdml file if generated, can be removed in the future when option to pipe gdml to the cli is possible
|
I adjusted the plotting to use I gave up on using the hist generated color bar since I could not get the z-label implemented. |
|
Are there any points not considered yet? |
ManuelHu
left a comment
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.
just some minor comments remaining for me
| new RMGIsotopeFilterScheme(); | ||
| new RMGTrackOutputScheme(); | ||
| new RMGParticleFilterScheme(); | ||
| new RMGGeomBenchOutputScheme(); |
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.
this doesn't even define commands, so you can remove this here and also the DefineCommands in RMGGeomBenchOutputScheme...?
ManuelHu
left a comment
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.
additional comments from clang-tidy:
/work/remage/src/RMGGeomBench.cc:26:1: warning: constructor does not initialize these fields: totalnevents, totalnpixels, npixelsperrow, neventsperpixel, cubesize, npixels_x, npixels_y, npixels_z, ID, starttime, currenttime, bunchstarttime, events_per_bunch, total_batch_rounds, current_batch_event, current_pixel_index, current_batch_round [cppcoreguidelines-pro-type-member-init]
26 | RMGGeomBench::RMGGeomBench() : RMGVGenerator("Benchmark") {
| ^
/work/remage/src/RMGGeomBench.cc:115:15: warning: redundant string initialization [readability-redundant-string-init]
115 | std::string targetvolumename = "";
| ^~~~~~~~~~~~~~~~~~~~~
| targetvolumename
/work/remage/src/RMGGeomBench.cc:226:19: warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
226 | int pixels_xz = npixels_x * npixels_z;
| ^
/work/remage/src/RMGGeomBench.cc:227:19: warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
227 | int pixels_yz = npixels_y * npixels_z;
| ^
/work/remage/src/RMGGeomBench.cc:228:19: warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
228 | int pixels_xy = npixels_x * npixels_y;
| ^
/work/remage/src/RMGGeomBench.cc:235:21: warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
235 | neventsperpixel = totalnevents / totalnpixels;
| ^
/work/remage/src/RMGGeomBench.cc:329:17: warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
329 | max_i = npixels_x;
| ^
/work/remage/src/RMGGeomBench.cc:330:17: warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
330 | max_j = npixels_z;
| ^
/work/remage/src/RMGGeomBench.cc:333:17: warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
333 | max_i = npixels_y;
| ^
/work/remage/src/RMGGeomBench.cc:334:17: warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
334 | max_j = npixels_z;
| ^
/work/remage/src/RMGGeomBench.cc:337:17: warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
337 | max_i = npixels_x;
| ^
/work/remage/src/RMGGeomBench.cc:338:17: warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
338 | max_j = npixels_y;
| ^
/work/remage/src/RMGGeomBench.cc:345:16: warning: variable 'x_pos' is not initialized [cppcoreguidelines-init-variables]
4 | double x_pos, y_pos, z_pos;
| ^
| = NAN
/work/remage/src/RMGGeomBench.cc:345:23: warning: variable 'y_pos' is not initialized [cppcoreguidelines-init-variables]
345 | double x_pos, y_pos, z_pos;
| ^
| = NAN
/work/remage/src/RMGGeomBench.cc:345:30: warning: variable 'z_pos' is not initialized [cppcoreguidelines-init-variables]
345 | double x_pos, y_pos, z_pos;
| ^
| = NAN
/work/remage/src/RMGGeomBench.cc:374:18: warning: variable 'median_time' is not initialized [cppcoreguidelines-init-variables]
374 | double median_time;
| ^
| = NAN
/work/remage/src/RMGGeomBench.cc:427:23: warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
427 | int events_so_far = ID;
| ^
/work/remage/src/RMGGeomBench.cc:428:25: warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
428 | current_batch_round = events_so_far / (totalnpixels * events_per_bunch);
| ^
/work/remage/src/RMGGeomBench.cc:429:19: warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
429 | int remainder = events_so_far % (totalnpixels * events_per_bunch);
| ^
/work/remage/src/RMGGeomBench.cc:438:30: warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
438 | int prev_pixel_index = (ID - 1) % (totalnpixels * events_per_bunch) / events_per_bunch;
| ^
/work/remage/src/RMGGeomBench.cc:458:19: warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
458 | int pixels_xz = npixels_x * npixels_z;
| ^
/work/remage/src/RMGGeomBench.cc:459:19: warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
459 | int pixels_yz = npixels_y * npixels_z;
| ^
/work/remage/src/RMGGeomBench.cc:461:7: warning: variable 'plane' is not initialized [cppcoreguidelines-init-variables]
461 | int plane;
| ^
| = 0
/work/remage/src/RMGGeomBench.cc:462:7: warning: variable 'i_index' is not initialized [cppcoreguidelines-init-variables]
462 | int i_index, j_index;
| ^
| = 0
/work/remage/src/RMGGeomBench.cc:462:16: warning: variable 'j_index' is not initialized [cppcoreguidelines-init-variables]
462 | int i_index, j_index;
| ^
| = 0
/work/remage/src/RMGGeomBench.cc:467:15: warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
467 | i_index = current_pixel_index % npixels_x;
| ^
/work/remage/src/RMGGeomBench.cc:468:15: warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
468 | j_index = current_pixel_index / npixels_x;
| ^
/work/remage/src/RMGGeomBench.cc:473:15: warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
473 | i_index = local_idx % npixels_y;
| ^
/work/remage/src/RMGGeomBench.cc:474:15: warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
474 | j_index = local_idx / npixels_y;
| ^
/work/remage/src/RMGGeomBench.cc:479:15: warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
479 | i_index = local_idx % npixels_x;
| ^
/work/remage/src/RMGGeomBench.cc:480:15: warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
480 | j_index = local_idx / npixels_x;
| ^
Co-authored-by: Manuel Huber <[email protected]>
Co-authored-by: Manuel Huber <[email protected]>
…gaion_benchmark_utilities
…ritzNeuberger/remage into navigaion_benchmark_utilities
Co-authored-by: Manuel Huber <[email protected]>
…ritzNeuberger/remage into navigaion_benchmark_utilities
… "X", "Y", "Z", and "Time" to "x", "y", "z", and "time" respectively.
gipert
left a comment
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.
I also have few minor comments. Thanks a lot for the work Moritz!
| ### Key metrics | ||
|
|
||
| **Mean navigation time** : Average time spent navigating to each point. Lower is | ||
| better. | ||
|
|
||
| **Max navigation time** : Slowest navigation time encountered. High values | ||
| indicate geometry hotspots. | ||
|
|
||
| **Standard deviation** : Variability in navigation time. High values suggest | ||
| non-uniform complexity. |
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.
I think this can be removed.
| 3. **Material boundaries**: Excessive material changes force more boundary | ||
| checks. Consolidate materials where physics allows. | ||
|
|
||
| 4. **Envelope optimization**: Proper envelope volumes can accelerate navigation |
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.
I think you don't explain well what you mean here
| - output_dir: Directory to store benchmark output files. | ||
| - num_events: Number of events to simulate. | ||
| - grid_increment: Increment between grid points in mm. | ||
| - grid_increments: Optional dict of increments per dimension in mm. | ||
| - dry_run: If set, only generate the macro file without running remage. |
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.
this does not render correctly in the API docs
| msg = f"{key}: {value}" | ||
| logger.info(msg) | ||
|
|
||
| except Exception as e: |
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.
is this try-catch block really needed?
|
|
||
| import pyg4ometry | ||
|
|
||
| ## GDML Handling Functions - Helper utilities for copying registry resources |
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.
This should become the module docstring
| Dictionary with keys: | ||
| - "object_lv": The loaded and renamed logical volume | ||
| - "registry": The pyg4ometry registry containing the geometry |
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.
this also does not render correctly
| Geometry Benchmark Analysis Results: | ||
| mean_navigation_time: 2.45 ns | ||
| median_navigation_time: 1.89 ns | ||
| std_navigation_time: 1.23 ns | ||
| max_navigation_time: 45.67 ns | ||
| min_navigation_time: 0.34 ns |
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.
is this the actual output?


This is an implementation of CJs benchmarking class referenced in #49.
Additionally, I added an optional output to report the median time per grid point.
Progress:
benchmark wrapper for remage as an additional cli
overlap identifier, i.e., check how many surfaces the Geantino passes. If not even, there must be an overlap [link] <-- necessary?