Adjust and document optical surface traversal#2345
Open
sethrj wants to merge 6 commits intoceleritas-project:developfrom
Open
Adjust and document optical surface traversal#2345sethrj wants to merge 6 commits intoceleritas-project:developfrom
sethrj wants to merge 6 commits intoceleritas-project:developfrom
Conversation
Test summary 5 701 files 9 238 suites 19m 37s ⏱️ Results for commit 667ed4f. ♻️ This comment has been updated with latest results. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2345 +/- ##
===========================================
- Coverage 87.26% 87.25% -0.01%
===========================================
Files 1377 1377
Lines 43836 43831 -5
Branches 13875 13872 -3
===========================================
- Hits 38253 38245 -8
- Misses 4524 4527 +3
Partials 1059 1059
🚀 New features to boost your workflow:
|
Member
Author
|
@hhollenb Could you please take a look at this? Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I was working on the optical ID refactor and hit the surprise that the "surface track position" is an opaque ID that didn't quite behave like an opaque ID. This PR is intended to reduce some of the confusion in "sub-surface" stepping by refactoring the data and IDs to behave more like IDs:
The analogy for these names is in ORANGE where they index into elements of a record. We do the same here for a single physics surface record.
See detailed documentation in https://github.com/sethrj/celeritas/blob/9899ff12805f601c1ce9d55c73aff07c3b934861/src/celeritas/optical/surface/SurfacePhysicsView.hh#L21 .
I have a feeling we can, in the future, entirely remove "orientation" and the sign swapping on surface normals, and turn those IDs into true IDs (not flipping them around). When entering a surface, we should be able to simply initialize the position and direction in the
{position N, negative dir}logical state and store{forward orientation = reported geometry normal < 0}as an additional logical state.