Skip to content

Remove temperature from interface - #49

Merged
RaulPPelaez merged 8 commits into
mainfrom
remove_temperature
Jun 27, 2025
Merged

Remove temperature from interface#49
RaulPPelaez merged 8 commits into
mainfrom
remove_temperature

Conversation

@rykerfish

Copy link
Copy Markdown
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
Comment thread tests/utils.py
@RaulPPelaez

RaulPPelaez commented Jun 13, 2025

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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.

@github-actions

github-actions Bot commented Jun 26, 2025

Copy link
Copy Markdown

Linter reported no issues

All Python files are correctly formatted with Black.

@github-actions

github-actions Bot commented Jun 26, 2025

Copy link
Copy Markdown

Linter reported no issues

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

Comment thread include/MobilityInterface/MobilityInterface.h
Comment on lines +191 to +192
// Compute the divergence of the mobility matrix,
// :math:`\boldsymbol{\partial}_\boldsymbol{x}\cdot

Copy link
Copy Markdown
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.

@RaulPPelaez RaulPPelaez left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
@rykerfish
rykerfish deleted the remove_temperature branch September 10, 2025 02:25
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