Add actions and executors for generating optical distribution data #1184
Conversation
- Add pre-step gather action to cache state data needed for pre-generators - Add post-step pre-generator action to create and buffer optical distribution data
amandalund
added
enhancement
There was a problem hiding this comment.
Good work! A couple pf general comments: 1) It is okay for now, but should actions for cerenkov and scintillation be separated as they may be independently added? 2) Do we need to add any unit test (okay if it is not necessary or difficult to add at this point though), especially for reproducibility, which I could not catch the requirement clearly from the code (so, please comment the place for the reproducibility requirement)? Thanks for the nice work!
| auto sim = track.make_sim_view(); | ||
| auto optmat_id | ||
| = track.make_material_view().make_material_view().optical_material_id(); | ||
| if (sim.status() == TrackStatus::inactive || !optmat_id |
There was a problem hiding this comment.
I don't think the track status will ever be inactive if the Executor is called: the TrackExecutor should filter out inactive tracks. If a track is inactive, then you might have just accessed invalid memory by getting the optical material ID...
There was a problem hiding this comment.
Isn't that only true if it's a conditional executor, i.e. constructed with make_active_track_executor()? The memory should still be valid even if the track is inactive. I used a regular TrackExecutor here because we need to clear any old distribution data in the buffers before compacting with remove_if.
There was a problem hiding this comment.
Oh ok, you're right about the conditional executor. But it still could access uninitialized memory right? If a track has never been initialized, then:
track.make_material_view().make_material_view().optical_material_id()could obtainan uninitialized/invalid material ID, and then crash when trying to read the optical material from the corresponding material view.
There was a problem hiding this comment.
Ahh ok I see what you mean: we'd be trying to construct a MaterialView with an invalid material ID. You're right, let me fix that...
There was a problem hiding this comment.
Yeah, I ran into a similar thing once with the cross section calculation.
|
Thanks @whokion: yes, we could separate the Cerenkov and scintillation actions, but this should also work as is either if both of the processes are enabled or if only one or the other is. I've added a test in #1173 (which I'll open once this is merged) which runs the stepping loop with these actions and checks the buffered data. For reproducibility the order of the distribution data in the buffers should always be the same, which is done by making sure the distributions created at each step are ordered by thread ID (originally I had used a stack allocator to buffer the data, but this would have required some additional post-processing to sort the distributions since the atomic add leads to a non-deterministic order: see #1173 (comment)). |
| detail::PreGenExecutor{properties_->host_ref(), | ||
| cerenkov_->host_ref(), | ||
| scintillation_->host_ref(), |
There was a problem hiding this comment.
Two actions would probably be cleaner, but if you think it's less effort to fix the single action let's go for that.
| auto optmat_id = inactive ? OpticalMaterialId{} | ||
| : track.make_material_view() | ||
| .make_material_view() | ||
| .optical_material_id(); |
There was a problem hiding this comment.
@amandalund I know this has been moved around (moved to Utils.hh in #1217) but wanted to revisit this on the original implementation... Since the PreGenExecutor is a "post-post-step" action, the material here is the post-boundary-crossing material if a track is stopped by a boundary. I don't think that's right... perhaps we should be collecting and storing the beginning-of-step optical material in OpticalPreStepData?
There was a problem hiding this comment.
Ah good catch, yeah we can store it with the rest of the pre-step data.
There was a problem hiding this comment.
@amandalund to be clear (and avoid unintentionally duplicating fixes as we've managed to do far too often 😅) would you like to address this or shall I?
There was a problem hiding this comment.
I'm happy to, if you haven't already done it 😉
There was a problem hiding this comment.
I haven't! I'd be grateful if you'd take handle it. Thanks!
This adds a gather action to cache the pre-step data needed for the scintillation and Cerenkov pre-generators and a post-step pre-generator action to create the optical distribution data. The distributions are stored in two large buffers indexed by the current size plus the track slot ID which is compacted at the end of each step by removing all the invalid distributions (this will ensure reproducibility). These actions will be created by the optical collector in #1173.