-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add an Optical WLS #1507
Add an Optical WLS #1507
Conversation
Test summary 3 877 files 5 969 suites 4m 9s ⏱️ Results for commit 390793f. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice work! I've got a couple of suggestions; if you want I can write the CDF sampler class.
@sethrj Thanks a lot for your review and comments. I addressed most of them, but left a couple of suggested improvements to you (a CDF sampler class and the integrate_trapezoid utility function). Sorry for asking additional help - I will be away for a week, but should catch up any other suggestion later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @whokion!
@sethrj can you help how to fix the compilation error from build-spack / orange-float-g4@11.0? Thanks. |
@whokion I've fixed the floating point error and added a couple of to-dos. Could you please take a look at the remaining discussion? |
…f there is no secondary photon generated from WLS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pending Seth's and Amanda's suggestions, I think it looks good!
@sethrj Thanks a lot for fixing the float issue and adding other notes. Besides the discussion for adding a model or models which can be revisited in the next MR, it would be great that we can merge this as it is. Thanks again! |
@amandalund If you're satisfied then please merge! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the new model! I've added the little "todo" tasks to #1263 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent, thanks @whokion!
Add an optical warelength shift (WLS) interactor and an associated test:
This is an initial implementation of an optical wavelength interactor based on the G4OpWLS process with a couple of differences: