-
Notifications
You must be signed in to change notification settings - Fork 34
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 muon decay interactor #1456
base: develop
Are you sure you want to change the base?
Conversation
Sampling loopThe
|
Test summary 3 385 files 5 208 suites 3m 53s ⏱️ Results for commit 4eaff7c. ♻️ This comment has been updated with latest results. |
= this->to_lab_frame(electron_nu_dir, electron_nu_energy, 0); | ||
auto muon_nu_4vec = this->to_lab_frame(muon_nu_dir, muon_nu_energy, 0); | ||
|
||
// \todo Return electron only? |
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.
Looking great @stognini! Still have to go over most of this, but since we were talking about this point on the call, I guess my preference would be to just create the electron secondary until we have a use case for the neutrinos. The extra bit of computation won't be a big deal, but the two extra secondaries per decay (that we can't track anyway) could increase the secondary/initializer memory requirements a bit. And since you've already implemented it and we'll have it in the history, we can easily just revert the change when we need it. (I'll let @sethrj and @whokion weigh in, though.)
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 do agree with @amandalund for now (unless we want to add neutrino-nucleus processes in a near-term future or want to send these neutrinos back to CPU so that Geant4 can handle them in the case that users add neutrino processes in their physics list, of which data transfer needs to be supported in a similar way when we add lepto-/photo-nuclear processes). Of course, please share any other users cases that you are considering.
Regarding to the maximum energy of the daughter electron from the muon decay, I think that the formulas in the P.M. is an approximation (i.e, assuming that secondaries are massless in the V-A calculation except the primary muon). The implementation in Geant4 may be more practically/experically correct. You may still recover the energy conservation in the rest frame of the muon following correct kinematics between (e + n_e) + n_muon and muon as necessary - I am surprised that Geant4 does not calculate them correctly. Anyway, I will also review and give more detailed comments later. Thanks for a good addition!
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.
@stognini Please address the first round of review comments. Thanks.
- Remove neutrinos from interactor/PDG namespace/test harness - Address most refactoring suggestions - TODO: replace for loops in favor of do whiles
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.
@stognini Thanks for the update. Please see the second set of comments (which are mostly minors).
* (real_type{1} - electron_nu_energy_frac)); | ||
|
||
electron_energy_frac = generate_canonical(rng); | ||
} while (electron_nu_energy_frac < real_type{1} - electron_energy_frac); |
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.
This may be safe without a loop check as the sampling is well-defined. Nevertheless, have you thought about combining two do-while loops into one (i.e., 2D sampling with a composition and rejection technique as their energies are correlated)?
// Momentum of secondaries at rest frame | ||
auto charged_lep_energy | ||
= this->calc_momentum(electron_energy_frac, shared_.electron_mass); | ||
|
||
// Apply a spherically uniform rotation to the decay | ||
auto sample_twopi = UniformRealDist(0, 2 * constants::pi); | ||
real_type euler_phi = sample_twopi(rng); | ||
real_type euler_theta = std::acos(UniformRealDist(-1, 1)(rng)); | ||
real_type euler_psi = sample_twopi(rng); | ||
|
||
EulerRotation rotate(euler_phi, euler_theta, euler_psi); | ||
Real3 charged_lep_dir = {0, 0, 1}; | ||
charged_lep_dir = rotate(charged_lep_dir); |
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.
As we are not creating neutrinos, we may need a comment for not calculating cos_theta
between the decay electron and its neutrinos (i.e., electron neutrino direction w.r.t electron). As discussed, I guess that the electron direction can be simply sampled by
charged_lep_dir = from_spherical(UniformRealDist(-1, 1)(rng), UniformRealDist(0, 2 * constants::pi));
without EulerRotation
.
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.
Looking good @stognini! A few small suggestions:
} while (generate_canonical(rng) | ||
> electron_nu_energy_frac | ||
* (real_type{1} - electron_nu_energy_frac)); |
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.
The RejectionSampler
could be used here (if the sampling is not refactored according to Soon's suggestion).
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.
So this is a weird detail that happens in the sampler that doesn't allow me to use any of the standard helper classes (that have reasonable assumptions, such as asserting that the acceptance value is non-negative). Here is the problem:
The electron_nu_energy_frac
range is [0, sample_max_)
, which is [0, 1.000023)
. Now, the f_
value set in the RejectionSampler
is f_ = electron_nu_energy_frac * (1 - electron_nu_energy_frac)
. So that means that for the upper limit (sample_max_ = 1.000023
), f_ < 0
, which leads to an assertion error.
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 agree we should try to refactor this. Do you know the underlying PDFs to be sampled? It would be really good to explicitly document those as well.
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 also don't think sampling the energy fraction in [0, sample_max
) makes sense... won't that be equivalent to sampling it in [0, 1), just slightly less efficient?
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.
Some thoughts on how this might be refactored: it looks like the inner rejection loop samples the
This PDF is also the beta distribution
Our GammaDistribution
implements the algorithm described in Marsaglia and Tsang which uses a rejection method with a very high (>95%) acceptance rate, but also has nested loops and draw samples from a normal distribution. For this simple case where we'd just need samples from
I think this would at least be more efficient than the current rejection method.
@stognini can you add a decay section to |
* This interactor follows \c G4MuonDecayChannel and the Physics Reference | ||
* Manual, Release 11.2, section 4.2.3. | ||
* | ||
* \warning Neutrinos are currently not returned in this interactor as they |
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 think this should be a note
... no one is going to be doing EM physics and neutrino physics inside the same calculation since the cross sections are so absurdly different.
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.
They won't be doing neutrino physics per se, but vertex quantities won't be conserved, and reconstruction/filtering for analyses might depend directly on verification of conservation of E and p to reconstruct a mother particle from an observed signal. So I thought it was reasonable to have a warning.
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.
Does Geant4 actually do this? Would the user have to have custom physics/stepper code to pull out the neutrinos and save their momentum and kill them?
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.
Geant4 just returns all three particles. So if the user doesn't want the neutrinos to be transported and/or use their initial state, then yes, the user would likely have that coded in their G4VUserTrackingAction
implementation.
{ | ||
return MevMomentum{ | ||
std::sqrt(ipow<2>(energy_frac * max_energy_) | ||
+ 2 * energy_frac * max_energy_ * mass.value())}; |
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.
Maybe this function should just be electron momentum? Not passing in the mass (just used shared_.electron_mass
) and instead passing in Energy{energy_frac * max_energy_}
.
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.
This was originally as is because it was the same function for all three secondaries. I kept it isolated for future refactoring if we need to add the neutrinos, but it could technically just be in the main interactor body, since it is just used once.
src/celeritas/phys/Interaction.hh
Outdated
@@ -35,6 +35,7 @@ struct Interaction | |||
{ | |||
scattered, //!< Still alive, state has changed | |||
absorbed, //!< Absorbed or transformed to another particle type | |||
decay, //!< N-body decay |
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 going to shoehorn the decay process into the regular "interactor", this is exactly equivalent to absorption, right? So let's just delete this and the from_decay
.
And in fact, you need to do this, because the InteractionApplier
tries to change the direction if
(result.action != Interaction::Action::absorbed)
, and it only kills the particle if it's absorbed.
So we should probably just delete this and use "from_absorption" . If you (or anyone else) think it's too confusing to mark it as absorbed
, then just use
decay, //!< N-body decay | |
decay = absorbed, //!< N-body decay |
and replace your function below with
CELER_FORCEINLINE_FUNCTION Interaction from_decay() { return from_absorption(); }
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.
But we still end up with result.action == absorbed
in this case, right? Because having it tagged as decay in our actions is important. For example: in a perfect vacuum, a particle will never undergo absorption, but it will always decay if not stable.
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.
Currently we don't store the interaction result in any way or provide a way for the user to interrogate it. It's only used to update the post-step data in the InteractionApplier. I think that's OK, since we should be able to use the post-step action ID for users to interrogate that sort of thing.
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.
Maybe we should change the name from absorbed
to killed
? But then that might be ambiguous with a particle that had an error and had to be killed.
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.
Sorry that I used a wrong account for the earlier comment by gxbert (now deleted). Recovering/rewording it here (but shorten): It is confusing that decay = absorbed
as the decay process is a material independent (in a sense that the particle never interacts with a material and never be physically absorbed). The action decay = absorbed will be ambiguous if the muon capture process is added later which can be considered as absorbed and physically totally different process even though multiple processes can be categorized to the same action. Nevertheless, a question (to @stognini ) is whether there is any specific treatment for decay products from others (for InteractionApplier) which may justify the addition.
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.
My initial enum addition was a starting point to expand the InteractionApplier
when the decay process class comes in, as well as its inclusion in the stepping loop on future PRs. So we would have a particle killed if it is absorbed or underwent decay. For now, our actions are internal, but I assume that at some point we will be able to provide access to actions to the end user, similar to querying processes in G4, for analysis purposes. So one could, for example, store only data from a given decay, if their focus is, say, understanding signals from a given channel.
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.
We can always query processes by looking at the post-step action ID, since each of those correlates to a model (decay or interaction) that the track undergoes. I don't think we'll want to expand that further. I think instead we may want to make the "interaction result" less about the physics and more about the programming implementation: whether to update the direction and/or kill the particle, e.g.
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.
Fair point. Should we rename absorbed
to a catch-all word, such as stopped
? This would cover all cases: scatters, stops (absorption, decay), remains unchanged or fails during sampling.
This PR adds the muon decay interactor. Muons can only decay via one channel, either
or
An important caveat is that the Geant4 physics manual defines the maximum possible sampled energy for the electron to be$$E_\text{max} = m_\mu / 2$$ , while the source code defines $$E_\text{max} = m_\mu / 2 - m_e$$ . This difference leads to a small missing energy, as the total energy of secondaries on a muon decay at rest is not equal to the muon rest energy in the case of the source code definition.