-
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
Support combined single and multiple Coulomb scattering #1230
Support combined single and multiple Coulomb scattering #1230
Conversation
- Add a new data/params class for Wentzel OK&VI data that will be used by both the single Coulomb scattering model and the Wentzel VI MSC model - Import MscThetaLimit from EM parameters instead of the model PolarAngleLimit
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.
Very good! Left some trivial comments just for clarification. Otherwise, good to go for now and hope that the WentzelVI msc model is available soon. We may also need more low/high-level tests/verification/validation for the Coulomb scattering, which belongs to me and @hhollenb. Thanks for the update.
Thanks @whokion for the helpful feedback! I agree about adding more testing, and I also still need to set the correct lower energy limit for the Coulomb scattering model, which I'm planning do in a follow-up PR. |
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.
That's a lot of work, thanks! Just have a couple small comments.
} | ||
if (!wentzel) | ||
{ | ||
// Set the minimum scattering angle for Coulomb single scattering |
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.
Is this :
// Set the minimum scattering angle for Coulomb single scattering | |
// Set the maximum scattering angle for Coulomb single scattering |
?
So for wentzel you can fully backscatter, but with coulomb scattering you can't change more than 90 degrees?
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.
It should be the maximum scattering angle for wentzel VI (180° if there's no single coulomb, or the msc theta limit from the EM params if there is) and the minimum scattering angle for coulomb (0° if there's no wentzel VI, else the msc theta limit).
One thing I'm still unsure about is the default value of the msc theta limit... from the literature (e.g. here) it sounds like it should be around 0.2 radians, but as far as I can tell in the EM params it's pi?
Thanks @amandalund ! @hhollenb Do you want to take a look at this before I 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.
Everything looks really good! With these changes it looks like I'm getting better agreement with Geant4 for just the single Coulomb scattering.
@@ -160,6 +160,17 @@ enum class MscStepLimitAlgorithm | |||
size_, | |||
}; | |||
|
|||
//---------------------------------------------------------------------------// | |||
//! Nuclear form factor model for Coulomb scattering | |||
enum class NuclearFormFactorType |
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.
If we're moving nuclear form factors to a more general namespace, should we also move their calculation out of WentzelDistribution
so there's a single implementation?
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.
I moved the enum here because we're now also using it in the import data structs, a lot of our other enums live here, and we shouldn't need additional changes for the SWIG build (#1159), but there wouldn't be any problem with leaving in the wentzel data. I think as long as the form factor calculations are only used in the wentzel distribution we can leave them there.
Thanks @hhollenb, that's great! |
This refactors and updates the single Coulomb scattering code in preparation for adding the Wentzel VI MSC model and combined single and multiple scattering. The main changes are: