Skip to content

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

Merged
merged 8 commits into from
Jun 27, 2025
Merged

Remove temperature from interface #49

merged 8 commits into from
Jun 27, 2025

Conversation

rykerfish
Copy link
Contributor

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 the temperature 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 had temperature=1.

Instead of rectifying this, Brennan and I thought it would be better to remove temperature from the interface entirely. This would make sqrtMdotW and thermalDrift return without the prefactors of $\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 but I'm currently taking it as an argument in hydrodynamicVelocities so the temperature-related prefactors can be added there.

However, hydrodynamicVelocities as a function is likely to be misleading. In most time integrators I think the $MF$ and $M^{1/2}$ terms will have different prefactors of dt on them, so we wouldn't be able to use hydrodynamicVelocities to do something like an Euler-Maruyama step without also taking in dt as a parameter. I suggest we either rename hydrodynamicVelocities to something like EM-step and take in a dt parameter as well so we can add it correctly, or just entirely remove it from the interface. This would be a slight performance hit to PSE, but might be worth it.

@RaulPPelaez what do you think? I summarized the main questions below since this got long.

  1. Removing temperature from the interface (no prefactors on sqrtMdotW and thermalDrift)
  2. What to do with hydrodynamicVelocities

@rykerfish rykerfish requested a review from RaulPPelaez June 13, 2025 00:14
@RaulPPelaez
Copy link
Contributor

RaulPPelaez commented Jun 13, 2025

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).
Thermal drift should then be renamed to something like "divM".
The hydrodynamicVelocities does: result = Mdot() + asqrtMdot() + bthermalDrift() (note that this function needs two prefactors now)
How do you call that? I would describe what it does as "sum the three terms"
Perhaps:

  • fullContribution
  • accumulatedTerms
  • composedTerms
    (I hate these)

…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.
@rykerfish
Copy link
Contributor Author

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 LangevinVelocities that takes in parameters for dt and kbt, as well as forces and torques, and computes velocities from the usual Langevin equation. This way, it's guaranteed to be dimensionally consistent and solves the naming problem at the same time. It's somewhat more narrow in scope that what you laid out, but I think it's mostly a convenience function anyway and this provides some safeguards and is easier to document.

Copy link

github-actions bot commented Jun 26, 2025

Linter reported no issues

All Python files are correctly formatted with Black.

Copy link

github-actions bot commented Jun 26, 2025

Linter reported no issues

All C/C++ files are correctly formatted with clang-format.

Comment on lines +191 to +192
// Compute the divergence of the mobility matrix,
// :math:`\boldsymbol{\partial}_\boldsymbol{x}\cdot
Copy link
Contributor

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.

Copy link
Contributor

@RaulPPelaez RaulPPelaez left a 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.

@RaulPPelaez
Copy link
Contributor

I bumped UAMMD to the latest version, which introduces compatibility with CUDA 12.9, also solves some anoying warnings.
I also fixed some tests requiring to be ran from within the tests folder by using relative folders.

@RaulPPelaez RaulPPelaez merged commit 5f5d8bf into main Jun 27, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants