Skip to content

Fix D3, Ewald, PME, DSF stress sign#571

Merged
CompRhys merged 3 commits into
TorchSim:mainfrom
teddykoker:d3-sign
Jun 6, 2026
Merged

Fix D3, Ewald, PME, DSF stress sign#571
CompRhys merged 3 commits into
TorchSim:mainfrom
teddykoker:d3-sign

Conversation

@teddykoker

@teddykoker teddykoker commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Summary

Checklist

Before a pull request can be merged, the following items must be checked:

  • Run ruff on your code.
  • Tests have been added for any new functionality or bug fixes.

@CompRhys

CompRhys commented Jun 6, 2026

Copy link
Copy Markdown
Member

Looks good, can you update the minimum pin to 0.3.1 which I think is the first release after that PR changing the convention.

I am unsure why the lowest test here of dispersion doesn't fail if this sign convention changed as the lowest pin should be downloading 0.3.0 which should have the convention opposite to your fix here.

@teddykoker teddykoker changed the title Fix D3 dispersion stress sign Fix D3, Ewald, PME, DSF stress sign Jun 6, 2026
@teddykoker

Copy link
Copy Markdown
Contributor Author

Ah I misread the alchemi PR above. The D3 code always used the negative derivative for virial, the change from 0.3.0 to 0.3.1 was updating the DSF and LJ modules to match. This means D3 always had the incorrect sign, but DSF was once correct and now wrong after 0.3.1. I bumped up the pin to this version and also fixed DSF, Ewald, and PME.

@CompRhys

CompRhys commented Jun 6, 2026

Copy link
Copy Markdown
Member

Okay I think then I had the wrong sign on everything thats for catching!

@CompRhys CompRhys merged commit 6c93642 into TorchSim:main Jun 6, 2026
60 of 64 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.

3 participants