Add KSIntSurfaceScattering to simulate scatterings on a surface - #147
Add KSIntSurfaceScattering to simulate scatterings on a surface#147richeldichel wants to merge 7 commits into
Conversation
This class adds the possibility to simulate scattering of an electron on a surface with a given probability for backscattering of the electron and a given probability for production of a secondary electron from the surface. Code originally by V. Hannen
There was a problem hiding this comment.
Pull request overview
This PR introduces a new surface interaction (KSIntSurfaceScattering) to model electron interactions with surfaces, including probabilistic backscattering and probabilistic secondary-electron emission, and wires it into the build system and XML bindings.
Changes:
- Added
KSIntSurfaceScatteringinteraction implementation and interface. - Added XML binding builder to configure the interaction via
ksint_surface_scatteringwith relevant attributes. - Registered new sources/headers in the Interactions and Bindings CMakeLists.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| Kassiopeia/Interactions/Source/KSIntSurfaceScattering.cxx | Implements the scattering interaction, including backscattering, absorption, and secondary creation logic. |
| Kassiopeia/Interactions/Include/KSIntSurfaceScattering.h | Declares the interaction API and configuration fields (probabilities, energies, side selection). |
| Kassiopeia/Interactions/CMakeLists.txt | Adds the new interaction source/header to the Interactions library build. |
| Kassiopeia/Bindings/Interactions/Source/KSIntSurfaceScatteringBuilder.cxx | Registers the new XML element and its attributes. |
| Kassiopeia/Bindings/Interactions/Include/KSIntSurfaceScatteringBuilder.h | Implements attribute-to-setter wiring for XML configuration. |
| Kassiopeia/Bindings/CMakeLists.txt | Adds the new builder source/header to the Bindings build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…eview Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Ran Scripts/gen-bindings-docs.sh to regenerate the documentation. The diff includes some other previous PRs.
|
I have added the suggestions by Copilot as they were all well-justified. Additionally, I enclosed the debug statements about the particle state in a The largest diff now comes from the update of the bindings documentation as I re-ran |
| tInitialNormalMomentum = -1.0 * tInitialNormalMomentum; //reverse direction for reflection | ||
| KThreeVector tInitialTangentMomentum = tInitialMomentum - tInitialNormalMomentum; |
There was a problem hiding this comment.
I get the impression at first glance that the next function has essentially the same code but programmed significantly more cleanly - most of my comments here don't apply there. Could it live in a common helper function GetReflectionMomentum(anInitialParticle, tSinTheta, tCosTheta) so it only exists once, preferably in the more clean way?
Other than that, I think there seems to be a bug here since this seems double-negating: tInitialNormalMomentum before the negation to my understanding should be the momentum in direction of the surface. Meaning tInitialMomentum - tInitialNormalMomentum before the negation should be the momentum orthogonal to tInitialNormalMomentum. That tInitialNormalMomentum = -1.0 * tInitialNormalMomentum; in combination with the minus sign in the next line does not make sense to me.
Also in general I think re-defining an existing variable is bad style. That also goes for the next two lines.
There was a problem hiding this comment.
This is covered already by your commit 68de7a4
There was a problem hiding this comment.
Yes, but do we need to follow up on the bug in the old code? We should ensure the old code is not used.
| if (tKineticEnergy < 0.0) { | ||
| intmsg(eError) << "surface diffuse interaction named <" << GetName() | ||
| << "> tried to give a particle a negative kinetic energy." << eom; | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
How would that happen? Isn't it better to ensure fScatLossFraction is between 0 and 1?
There was a problem hiding this comment.
Replaced with a single check in ExecuteInteraction: fScatLossFraction must be in [0, 1) (strictly less than 1, to avoid a zero-kinetic-energy particle hitting the NaN-momentum path in KSParticle::SetKineticEnergy).
There was a problem hiding this comment.
Not really sure why this was moved from ExecuteReflection to ExecuteInteraction - I would always check variables as close to where they are used as possible. But I'd also be fine with it this way.
Implemented suggested improvements
…d address remaining review comments
|
Thanks for your suggestions and helping with the implementation @2xB! I have updated the PR and will try to reply to your comments individually. If it is good from your side, I would try to get some feedback from our group on whether the simulation runs as expected and then merge it. |
This class adds the possibility to simulate scattering of an electron on a surface with a given probability for backscattering of the electron and a given probability for production of a secondary electron from the surface.
Code originally by V. Hannen