Add optical sensitive detectors#2215
Add optical sensitive detectors#2215hhollenb wants to merge 12 commits intoceleritas-project:developfrom
Conversation
hhollenb
added
enhancement
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #2215 +/- ##
===========================================
+ Coverage 86.29% 86.30% +0.01%
===========================================
Files 1316 1323 +7
Lines 41605 41711 +106
Branches 12835 12853 +18
===========================================
+ Hits 35902 36000 +98
- Misses 4506 4515 +9
+ Partials 1197 1196 -1
🚀 New features to boost your workflow:
|
Test summary 5 825 files 9 348 suites 18m 19s ⏱️ Results for commit 9300f46. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
This PR adds a simple first implementation of optical sensitive detectors for scoring photon hits, addressing issue #1397. The implementation uses existing detector regions from DetectorParams to define where hits should be recorded. Hits are copied for every track into a state buffer, with tracks not in detector regions marked with invalid detector IDs. All hits are transferred to pinned memory on the host, cleaned up, and passed to a user callback function.
Changes:
- Added optical detector scoring infrastructure with
ScoringParams,DetectorAction,ScoringTrackView, and related data structures - Integrated scoring into the optical core parameters and track initialization
- Added comprehensive tests for hit recording with both small and large numbers of photons
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/geocel/data/optical-box.gdml | Modified GDML geometry to add 5 surrounding detector volumes instead of optical surfaces |
| test/celeritas/optical/SurfacePhysicsIntegrationTestBase.cc | Removed default surface setup that's no longer needed |
| test/celeritas/optical/OpticalMockTestBase.hh | Added stub for build_optical_scoring |
| test/celeritas/optical/Detector.test.cc | New comprehensive test file with simple and stress tests for detector scoring |
| test/celeritas/OnlyCoreTestBase.hh | Added unreachable stub for build_optical_scoring |
| test/celeritas/ImportedDataTestBase.hh/cc | Added implementation of build_optical_scoring that creates empty scoring params |
| test/celeritas/GlobalTestBase.hh/cc | Added optical_scoring accessors and virtual detector() override support |
| test/celeritas/CMakeLists.txt | Registered new detector tests |
| src/celeritas/setup/Problem.cc | Integrated ScoringParams creation into optical problem setup |
| src/celeritas/optical/detector/ScoringTrackView.hh | New track view for accessing and managing hit data per track |
| src/celeritas/optical/detector/ScoringParams.hh/cc | New params class that manages user callback and registers DetectorAction |
| src/celeritas/optical/detector/DetectorExecutor.hh | New executor that copies hit data for all tracks at end of step |
| src/celeritas/optical/detector/DetectorData.hh/cc/cu | New data structures for hits and state, plus host/device copy functions |
| src/celeritas/optical/detector/DetectorAction.hh/cc/cu | New post-step action that executes detector executor and processes hits |
| src/celeritas/optical/CoreTrackView.hh | Added scoring() accessor and initialization of hit data |
| src/celeritas/optical/CoreTrackData.hh/cc | Added DetectorStateData to core state and resize logic |
| src/celeritas/optical/CoreParams.hh | Added ScoringParams member and accessor |
| src/celeritas/inp/Scoring.hh | Added OpticalScoring struct with hit callback function |
| src/celeritas/inp/Problem.hh | Added scoring field to OpticalProblem |
| src/celeritas/CMakeLists.txt | Added new source files to build |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #include <iostream> | ||
|
|
||
| namespace celeritas | ||
| { | ||
| namespace optical |
There was a problem hiding this comment.
The iostream header is included but not used in this file. This should be removed to avoid unnecessary includes.
| #include <iostream> | |
| namespace celeritas | |
| { | |
| namespace optical | |
| namespace celeritas | |
| { | |
| namespace optical | |
| namespace optical |
| /*! | ||
| * Copy hits from host state data to pinned memory. | ||
| * | ||
| * Because both buffer reside host-side, this is just a trivial copy between |
There was a problem hiding this comment.
Spelling error: "reside" should be "reside". The comment should read "Because both buffers reside host-side" not "Because both buffer reside host-side".
| * Because both buffer reside host-side, this is just a trivial copy between | |
| * Because both buffers reside host-side, this is just a trivial copy between |
| * Process hits copied from the kernels and send them to the callback. | ||
| * | ||
| * Copied hits might be invalid, and are removed before sending into the | ||
| * callback function. The callback is only execute when a non-zero amount of |
There was a problem hiding this comment.
Spelling error: "execute" should be "executed". The comment should read "The callback is only executed when a non-zero amount of valid hits occurs."
| * callback function. The callback is only execute when a non-zero amount of | |
| * callback function. The callback is only executed when a non-zero amount of |
| class ScoringParams final | ||
| { | ||
| public: | ||
| //!@{ | ||
| //! \name Type aliases | ||
| using HitCallbackFunc = inp::OpticalScoring::HitCallbackFunc; | ||
| //!@} | ||
|
|
||
| public: |
There was a problem hiding this comment.
The naming scheme between "Scoring" and "Detector" terminology is potentially confusing. The PR description mentions this as a known issue. Consider:
DetectorStateDatais accessed via the "scoring" member inCoreStateDataScoringParamsmanagesDetectorActionScoringTrackViewoperates onDetectorStateData
For clarity and consistency, consider either:
- Renaming to use "Detector" consistently (e.g.,
DetectorScoringParams,DetectorTrackView) to clarify these are specifically for detector scoring - Renaming to use "Scoring" consistently for the state and views
- Using "Hit" terminology more consistently (e.g.,
HitScoringParams,HitStateData,HitTrackView)
The current mixed terminology could lead to confusion about which components relate to detector geometry vs. hit scoring.
sethrj
left a comment
sethrj
left a comment
There was a problem hiding this comment.
Sorry this took me a minute to review. I have just a couple of thoughts, most of them with naming. Perhaps we could do a quick hackathon today/tomorrow to get the names all lined up across the codebase?
| ParticleStateData<W, M> particle; | ||
| PhysicsStateData<W, M> physics; | ||
| RngStateData<W, M> rng; | ||
| DetectorStateData<W, M> scoring; |
| CELER_FUNCTION void ScoringTrackView::score_hit(DetectorHit hit) | ||
| { | ||
| CELER_EXPECT(hit.detector); | ||
| this->hit() = std::move(hit); |
There was a problem hiding this comment.
Since move can't be used on device (and the data is all trivial) let's just send the argument by const reference and make a copy here.
| using StateItems = StateCollection<T, W, M>; | ||
| //!@} | ||
|
|
||
| StateItems<DetectorHit> all_track_hits; |
There was a problem hiding this comment.
| StateItems<DetectorHit> all_track_hits; | |
| StateItems<DetectorHit> detector_hits; |
for consistency with other state data objects in the code
| * back to the user provided callback function. If the callback function is not | ||
| * provided, then no \c DetectorAction is created. | ||
| */ | ||
| class ScoringParams final |
There was a problem hiding this comment.
We don't mark as final any of the other classes that don't inherit from an interface
| * where invalid hits are erased. A span of only valid hits is then passed into | ||
| * the user provided callback function. | ||
| */ | ||
| class DetectorAction final : public OpticalStepActionInterface, |
There was a problem hiding this comment.
I think to be consistent these should all be ScoringAction/Data/Executor... we might want to defer renaming to a different PR though, or at least lay out what the
I was also confused initially because DetectorAction is just an implementation detail of ScoringParams, and the creation of those is what does all the work (kind of like the optical offload). The action/executor definitely need to be in a detail namespace.
There was a problem hiding this comment.
Looking back on it, I think scoring is a bad name cause that is usually used for the simpler tallying of things like energy deposits. Maybe a DetectorHitParams or UserDetectorParams is better for indicating that it's meant to deal with the actual hits and callbacks?
There was a problem hiding this comment.
Don't think too hard about it; we could easily end up in a hell of combining detector, sensitive, hit, callback, user, score, tally, ...
I think "sensitive detector" is a special case of a "scoring region": the scoring region is specifically a set of logical volumes, and the scoring action is a general "callback" capability that gets the entire step.
An arguably scoring regions are just a specialization of a "user stepping action"...
3 participants
Add optical sensitive detectors for scoring photon hits for #1397
As a simple first implementation of scoring hits, the same detectors from
DetectorParamsis used to define the detector regions. TheDetectorExecutorkernel is responsible for copying hits for every track into the state buffer, with tracks not in detector regions marking their hits with an invalid detector ID. All hits are copied into pinned memory on the host, which is then cleaned up and passed to a user callback function.This isn't an optimized / complete implementation by any means, but I've added some tests to help check future optimizations are correct.
I ran into some collisions with naming
ScoringParamsandDetectorParamsso I just wrote something quickly and I'm seeking suggestions for what to rename things to.