-
Notifications
You must be signed in to change notification settings - Fork 2
Remove temperature from interface #49
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
Conversation
Agree with everything except for the rename of hydrodynamic velocities as long as it is well documented. The other methods are really clear on what they do and removing the temperature makes sense (its called "sqrtMdot", without kT).
|
…MdotW and divM so there are no longer hidden scalars. rewrote hydrodynamicVelocities and called it LangevinVelocities, which now take in the necessary arguments (temperature and dt) to ensure a dimensionally consistent result & computes velocities from a Langevin equation.
…datory but the forces and torques are not
The idea of hydrodynamicVelocities with two arbitrary prefactors somewhat scared Brennan and I because of the complete lack of dimensional consistency that could arise from a naive user. We also thought it wouldn't be particularly useful since most time stepping methods that are more complicated than Euler-Maruyama require computations at different positions (e.g. midpoint), and I don't think something even like a stochastic Adams-Bashforth would be particularly easy since the stochastic piece isn't included from the previous timestep. I went for |
✅ Linter reported no issues All Python files are correctly formatted with Black. |
✅ Linter reported no issues All C/C++ files are correctly formatted with clang-format. |
// Compute the divergence of the mobility matrix, | ||
// :math:`\boldsymbol{\partial}_\boldsymbol{x}\cdot |
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.
Should we document these the right way with Doxygen, even when we are not rendering them at all in the docs?
Perhaps we should do so and render them in the "How to add a new solver" page. I will take care of it in another 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.
Looks good to me, please consider my comments but feel free to merge as is.
I bumped UAMMD to the latest version, which introduces compatibility with CUDA 12.9, also solves some anoying warnings. |
I found a bug where the default implementation for
sqrtMdotW
was not accounting for the temperature in the interface. This affected NBody and DPStokes since they rely on Lanczos, but did not affect PSE or SelfMobility since they have their own implementations. I discovered this since changing thetemperature
parameter in the fluctuation-dissipation tests only made SelfMobility and PSE fail, while the other solvers continued to pass since they acted as if they hadtemperature=1
.Instead of rectifying this, Brennan and I thought it would be better to remove$\sqrt{2 k_BT}$ and $k_BT$ , respectively. We like this more since a hidden always-there prefactor on some functions is a bit counter-intuitive and created this bug in the first place. I removed
temperature
from the interface entirely. This would makesqrtMdotW
andthermalDrift
return without the prefactors oftemperature
from the interface but I'm currently taking it as an argument inhydrodynamicVelocities
so the temperature-related prefactors can be added there.However,$MF$ and $M^{1/2}$ terms will have different prefactors of
hydrodynamicVelocities
as a function is likely to be misleading. In most time integrators I think thedt
on them, so we wouldn't be able to usehydrodynamicVelocities
to do something like an Euler-Maruyama step without also taking indt
as a parameter. I suggest we either renamehydrodynamicVelocities
to something likeEM-step
and take in adt
parameter as well so we can add it correctly, or just entirely remove it from the interface. This would be a slight performance hit toPSE
, but might be worth it.@RaulPPelaez what do you think? I summarized the main questions below since this got long.
sqrtMdotW
andthermalDrift
)hydrodynamicVelocities