Skip to content
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 switching function for Coulomb prior #287

Merged
merged 3 commits into from
Feb 20, 2024
Merged

Conversation

peastman
Copy link
Collaborator

Originally I scaled the Coulomb prior by $\text{erfc}(\alpha r)$ to reduce its effect at short distances. It turned out this didn't work well because it was still too short ranged. Even at distances of only 1-2 A there was already a large component of Coulomb energy. I changed it to a cosine switching function which works much better.

@peastman
Copy link
Collaborator Author

I think the test failure was caused by #286. It's also failing on the main branch.

@stefdoerr
Copy link
Collaborator

Fixed in main now

@RaulPPelaez
Copy link
Collaborator

Looks good to me. This invalidates checkpoints using the Coulomb prior. I get that it is for the better but it is starting to happen a lot.
We really need to cook versioning into the checkpoints...

@RaulPPelaez
Copy link
Collaborator

I added the versioning functionality in #288

@peastman
Copy link
Collaborator Author

I don't think anyone has ever used the Coulomb prior. It just didn't work very well. I wouldn't worry about breaking compatibility with it.

@peastman
Copy link
Collaborator Author

Tests are passing now.

Copy link
Collaborator

@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.

LGTM Thanks Peter

@RaulPPelaez RaulPPelaez merged commit 166b7db into torchmd:main Feb 20, 2024
2 checks passed
@peastman peastman deleted the coulomb branch February 20, 2024 20:57
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