Simplify and fix safety/MSC

[sethrj]

As we discussed, the original find_safety track function was slightly over-engineered: we only ever calculate the safety from the current position. Simplify the interfaces for find_safety as well as the UrbanMscStepLimit class.

---

Additionally, prevent negative safety distances from VecGeom (this was apparently happening: we need more assertions!) and update UrbanMscStepLimit so that if we do get negatives they'll implicitly be treated like zeros.

[whokion]

This branch is compiled okay, but has a runtime issue from the demo-loop:

#0 0x000000000269ef20 in celeritas::device_debug_error ()
#1 0x00000000027c0c50 in __nv_static_63__50_tmpxft_00011844_00000000_7_AlongStepAction_cpp1_ii_16c32506__ZN9celeritas9generated74_GLOBAL__N__50_tmpxft_00011844_00000000_7_AlongStepAction_cpp1_ii_16c3250617along_step_kernelENS_7CoreRefILNS_8MemSpaceE1EEE<<<(16,1,1),(256,1,1)>>> ()

[whokion]

In general, this approach should be a correct assumption if the safety is always akin to the current geometry state. However, how can I find the safety in the previous position (just as a user case) unless it is saved as a cached data? - So, should we add the safety at the last calculated position too? The safety in UrbanMscStepLimit should be the safety in the pre-step position while the safety in UrbanMscScatter should be calculated at the updated position after the field propagator - Nevertheless, the proposed implementation should be okay as we do not update the geometry after the range limiter.

Anyway, the MR is good to merge once the demo-loop issue is resolved.

[whokion]

Drop 'a'

[whokion]

This branch is compiled okay, but has a runtime issue from the demo-loop:

#0 0x000000000269ef20 in celeritas::device_debug_error () #1 0x00000000027c0c50 in __nv_static_63__50_tmpxft_00011844_00000000_7_AlongStepAction_cpp1_ii_16c32506__ZN9celeritas9generated74_GLOBAL__N__50_tmpxft_00011844_00000000_7_AlongStepAction_cpp1_ii_16c3250617along_step_kernelENS_7CoreRefILNS_8MemSpaceE1EEE<<<(16,1,1),(256,1,1)>>> ()

Apparently, the following line in UrbanMscStepLimit is causing the problem;
CELER_EXPECT(safety_ >= 0);

[sethrj]

In general, this approach should be a correct assumption if the safety is always akin to the current geometry state.

Yes I should be.

However, how can I find the safety in the previous position (just as a user case) unless it is saved as a cached data? - So, should we add the safety at the last calculated position too?

I think if users need a "previous position" safety then we should work with them to cache it as part of their workflow... if it's not always needed, then people who don't need it shouldn't have to pay for it.

The safety in UrbanMscStepLimit should be the safety in the pre-step position while the safety in UrbanMscScatter should be calculated at the updated position after the field propagator - Nevertheless, the proposed implementation should be okay as we do not update the geometry after the range limiter.

Yes, since the geometry pos is updated during propagation it should be this way

Anyway, the MR is good to merge once the demo-loop issue is resolved.

Ugh, it looks like we never previously checked that the safety was nonnegative... so something in VecGeom (I assume that' you have it enabled?) is giving us negative safety distances... I'll check it out.

[sethrj]

@whokion Shall I also add to this PR the fix for the "increment step" being in the wrong place?

[whokion]

@sethrj There is a test failure from UrbanMsc.test.cc (i.e., celeritas_em_UrbanMsc) due to the negative safety protection (so sampling changes) which needs to be addressed before merging - my msc validation test with respect to Geant4 TestEM15 (which has not been committed yet) looks good with this MR as it statistically compare results between Geant4 and celeritas, so this MR itself should be good to merge.

[whokion]

@whokion Shall I also add to this PR the fix for the "increment step" being in the wrong place?

Yes, please. Thanks.

[sethrj]

The test failure is because on_boundary_ is different from safety_ > 0 (because "on boundary" can also mean "first step"). @whokion which is more correct?

    if (result.true_path < shared_.params.limit_min_fix()
        || (!on_boundary_ && distance < safety_))
    {
        result.is_displaced = false;
        auto temp           = this->calc_geom_path(result.true_path);
        result.geom_path    = temp.geom_path;
        return result;
    }

or

    if (result.true_path < shared_.params.limit_min_fix()
        || (safety_ > 0 && distance < safety_))
    {
        result.is_displaced = false;
        auto temp           = this->calc_geom_path(result.true_path);
        result.geom_path    = temp.geom_path;
        return result;
    }

[whokion]

I guess that the later should be closer to the original (Gean4) logic.

[sethrj]

So then the difference is that this conditional is about the physical distance from the boundary, whereas the other flag is about entering a new volume? Instead of safety_ > 0 || first_step for the on_boundary_ logic, should we be cacheing the previous material ID and comparing against the current material?

[whokion]

Also the current definition of on_boundary_ which includes the first step is also miss-leading as the vertex can be anywhere.

[whokion]

So then the difference is that this conditional is about the physical distance from the boundary, whereas the other flag is about entering a new volume? Instead of safety_ > 0 || first_step for the on_boundary_ logic, should we be cacheing the previous material ID and comparing against the current material?

Well, the material ID can be same for different logical/physical volumes.

[whokion]

I am not sure whether this is the right place to increase the step counter since any of along step actions (either
range limiter or msc) may not be selected for the process governing the current step - well the range limiter is a part of pre-step though in our implementation. So, should it be increased either when msc is selected, or inside somewhere discrete_select_track if msc (and the range limiter?) is not select.

[sethrj]

The end of "along step" seems like the right place to step counter, right? Any track that is active needs to have its step counter incremented, and (aside from the "zero step" special case where we now increment it) this will be called for every active track.

[whokion]

To me, places where we should increase the step counter (exclusively) are 1) when msc is selected, i.e, inside the block and 2) when the geometry limited step is selected, i.e, inside the block, and 3) the inside discrete_select_track. Of course, it also need to check whether either msc or geometry limited step is selected at the line. So, not sure whether putting the increment_num_steps() at the end of along step is equivalent to the logic above, i.e, make sure whether the step counter is neither over counted nor under counted, but increased once and only once per alived step.

[sethrj]

I think the end of AlongStep is going to be the cleanest place to put it: it's the only place (barring the one "return if zero step length") that is going to be guaranteed to be called before the final particle processing... having to put it in multiple different kernels risks forgetting to put it somewhere. Adding it to more locations (such as when the step limiter is selected, which at that point can still be overridden by slowing down) increases the risk of errors.

I guess it really matters most depending on what actually uses the step counter: and since the only things right now are the diagnostics and the MSC "first step" exception, and nothing in physics (or boundary crossing) does. So let's leave it here as the last thing that happens in "along step", and we can make sure it's documented and can later change it as needed.