-
Notifications
You must be signed in to change notification settings - Fork 29
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
Mott scattering #66
base: main
Are you sure you want to change the base?
Mott scattering #66
Conversation
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 PR and for providing documentation with the code! I have some comments that need to be addressed before merging.
} | ||
} | ||
|
||
/* ROOT TF1 takes in analytical formulas as strings. This is the conversion into strings */ |
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 there any reason why this is implemented via TF1
and not as C++ code directly? All the functions are available in C++ STL too, so I don't see why you'd need ROOT.
In my experience the formula parsing in ROOT adds considerable overhead and degrades performance. For code that is executed multiple times during simulation, I would not recommend this approach.
double KSIntCalculatorMott::GetEnergyLoss(const double& anEnergy, const double& aTheta){ | ||
double M, me, p, anELoss; | ||
|
||
M = 4.0026 * katrin::KConst::AtomicMassUnit_eV(); // eV/c^2 |
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 it would be useful to make the target mass M
a user parameter that can be changed in the XML file.
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 calculator only works for Helium, so I guess the name is a bit misleading. Maybe I can change the name to he_mott_calculator?
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 is a good point! Would you also rename the classes and filenames then?
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.
Yes, I will update class names and filenames all together after I make the appropriate changes into a sole c++ implementation. I have to use rejection sampling to sample from the cross section, but I have never done this before so it might take me quite a bit of time. I originally used the TF1 class to shortcut this step, but using c++ will improve the performance as the other comment says
Is there any update on this PR @juniorpena ? |
Hi,
No updates as of now. Other projects came to the forefront, and I still have not gone back to update this code. I will probably not get back to this until sometime closer to the summer.
Best,
Junior
From: J B ***@***.***>
Reply-To: KATRIN-Experiment/Kassiopeia ***@***.***>
Date: Monday, January 9, 2023 at 3:29 AM
To: KATRIN-Experiment/Kassiopeia ***@***.***>
Cc: Junior Ivan Pena ***@***.***>, Mention ***@***.***>
Subject: Re: [KATRIN-Experiment/Kassiopeia] Mott scattering (PR #66)
Is there any update on this PR @juniorpena<https://github.com/juniorpena> ?
—
Reply to this email directly, view it on GitHub<#66 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ARHKF652ECFCVFXCX6NP3TDWRPD6FANCNFSM57JDPJXQ>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Hello, I've eliminated the use of ROOT in this scattering calculator. Now the GetTheta function uses inverse transform sampling and rejection sampling. I've also updated it to allow for both He and Ne Mott Scattering |
Hey, thank you very much! Could you add documentation of the added objects to https://github.com/KATRIN-Experiment/Kassiopeia/blob/main/Kassiopeia/XML/Complete/Interactions.xml ? That is helpful both for users and for the review. (Related, but independent issue: #106 ) |
I've added the documentation |
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.
After looking through the code, I had three remaining comments after which this looks good to merge!
Btw. if there will be a thesis describing a use case of this, one can also advertise it in the top comment of KSIntCalculatorMott.h
, since it is always useful to see what the original use case for components of Kassiopeia was.
if (aContainer->GetName() == "theta_min") { | ||
aContainer->CopyTo(fObject, &KSIntCalculatorMott::SetThetaMin); | ||
|
||
if ((fObject->GetThetaMin() == 0.0)) { |
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 ((fObject->GetThetaMin() == 0.0)) { | |
if ((fObject->GetThetaMin() <= 0.0)) { |
std::vector<double> theta_arr; | ||
|
||
// Populate theta_arr | ||
for (int i = 0; i <= 1000; ++i) { | ||
theta_arr.push_back(fThetaMin + i * (fThetaMax - fThetaMin) / 1000.0); | ||
} |
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.
For a fixed-size array, std::array
is better than std::vector
. However, in this case, I think the array is not needed, as you just build it and directly afterwards use it - one could just calculate theta on the fly in the second loop. Also, it makes sense to document in a comment where the 1000 comes from and when it is valid?
amu = 4.0026; | ||
} else if (fNucleus == "Ne") { | ||
amu = 20.1797; |
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.
Please add a source for such numbers so one can more easily understand and update them (there are many hard-coded numbers besides these two further down as well).
This change is implementing Mott scattering of high energy electrons off of Helium nuclei. I uploaded the files from my Kassiopeia setup on my local machine into a repo I forked, and I made changes to the appropriate CMakeLists.txt files.