-
Notifications
You must be signed in to change notification settings - Fork 200
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
Use KineticEnergy instead of getEnergy #5786
Use KineticEnergy instead of getEnergy #5786
Conversation
Thanks for the clean-up @lucafedeli88! We do need to force double precision for the cases where |
I see!
Double precision can be forced like that: |
const amrex::ParticleReal ux, const amrex::ParticleReal uy, const amrex::ParticleReal uz, | ||
const amrex::ParticleReal mass) | ||
T KineticEnergy( | ||
const T ux, const T uy, const T uz, const T mass) |
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 particle velocity components would be of type ParticleReal
. Won't this cause a problem since it now expects all inputs to be of the templated type T
(which is specified to be double
)?
Could this be:
const T ux, const T uy, const T uz, const T mass) | |
const amrex::ParticleReal ux, const amrex::ParticleReal uy, const amrex::ParticleReal uz, const T mass) |
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 fine if you force double
precision, since it would be either a no-op or a promotion. But you are right, it could be an issue if for some reason amrex::ParticleReal
is double
and you try to force single precision.
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 could be a solution that avoids this issue:
/**
* \brief Computes the kinetic energy of a particle.
* This method should not be used with photons.
*
* @tparam T floating-point type to be used in internal calculations (by default equalt to amrex::ParticleReal)
* @param[in] ux x component of the particle momentum (code units)
* @param[in] uy y component of the particle momentum (code units)
* @param[in] uz z component of the particle momentum (code units)
* @param[in] mass mass of the particle (in S.I. units)
*
* @return the kinetic energy of the particle (in S.I. units)
*/
template <typename T = amrex::ParticleReal>
AMREX_GPU_HOST_DEVICE AMREX_INLINE
T KineticEnergy(
const amrex::ParticleReal ux, const amrex::ParticleReal uy, const amrex::ParticleReal uz, const amrex::ParticleReal mass)
{
using namespace amrex;
constexpr auto one = static_cast<T>(1.0);
constexpr auto inv_c2 = one/Math::powi<2>(static_cast<T>(PhysConst::c));
// The expression used is derived by reducing the expression
// (gamma - 1)*(gamma + 1)/(gamma + 1)
const auto u2 = static_cast<T>(ux*ux + uy*uy + uz*uz);
const auto gamma = std::sqrt(one + u2*inv_c2);
return one/(one + gamma)*static_cast<T>(mass)*u2;
}
T
is by default equal to ParticleReal
. However, you can override this and then calculations are carried out with precision T
and the result is of type T
. Conversions are carried out using static_cast
, so it should work ok.
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 code cleaning!
We currently have two functions in the code that do almost exactly the same thing:
and
The only differences are that
getEnergy
forces the use of double precision and returns an energy ineV
. I think that it would be better if we could use only one function for clarity. Therefore, in this PR I replacedgetEnergy
withKineticEnergy
.The units of the output are easy to deal with (we can just divide by 1eV the result). The precision is trickier. @roelof-groenewald , do you think that we really need to force
double
precision here? If it is the case, I think that it could be preferable to makegetEnergy
a template function and force double precision with something likeKineticEnergy<double>(...)
. What do you think?