Leverage particle.move_distance in event advance #3544
Conversation
GuySten
requested review from
paulromano and
pshriwise
and removed request for
pshriwise
| std::min({boundary().distance(), collision_distance(), distance_cutoff}); | ||
|
|
||
| // Advance particle in space and time | ||
| // Short-term solution until the surface source is revised and we can use |
There was a problem hiding this comment.
To close the loop here, do we know if there is an issue or PR related to this comment we can cite in this PR that resolved the surface source problem?
There was a problem hiding this comment.
This comment was added in #2631. You can read the discussion there but it was not clear to me what the problem was or why now it's working.
There was a problem hiding this comment.
Me either, but the behavior should be equivalent so I won't let it hold up this PR.
There was a problem hiding this comment.
Maybe the intent was to include time updates inside move_distance. @cfichtlscherer, can you provide some extra info here?
There was a problem hiding this comment.
Sorry, can't remember why we had it in a loop, but looks good for me.
There was a problem hiding this comment.
There was a surface source test that was failing when the change to use move_distance was made. It was unclear at the time why -- probably some weirdness with how the compiler was optimizing code resulting in slightly different numeric results. Whatever the reason was, there have evidently been enough changes (compilers, tests, implementation, underlying libraries) that it is no longer affected as evidenced by the tests on this PR passing.
Here was my original comment about it:
#2454 (comment)
4 participants
Description
This PR simplifiy event advance by using particle.move_distance.
Checklist
I have followed the style guidelines for Python source files (if applicable)I have made corresponding changes to the documentation (if applicable)I have added tests that prove my fix is effective or that my feature works (if applicable)