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

Create interface to md_tian2 potentials #28

Draft
wants to merge 32 commits into
base: main
Choose a base branch
from

Conversation

jamesgardner1421
Copy link
Member

Here is your new PR @clbox. I will make a couple of modifications before we merge it.

@jamesgardner1421
Copy link
Member Author

I've added a test now that I was able to run locally but obviously won't run in the CI because the library is missing. Ideally I would like this to work using BinaryBuilder to provide the cross platform library automatically so we could run this on Github. Unfortunately BinaryBuilder has to use the gfortran compiler which doesn't work with md_tian2 because they use a few ifort specific features, specifically the directory keyword for the inquire intrinsic. So we would have to fork the repository and patch anything that doesn't work with gfortran. Ultimately this would be a lot of hassle for minimal gain.

Another option is to set up a new test job that will download and compile md_tian2 before running the tests. This might be easier but again I'm not sure about the availability of ifort in Github actions. If we do want to do either of these probably we should move this code to a separate package as we've done previously with other extension models. This would make it easier to set up alternative testing infrastructure or additional binary dependencies. As it is I don't feel too great about including an interface that cannot be included in the automated tests in the main package.

@jamesgardner1421 jamesgardner1421 marked this pull request as draft November 21, 2022 21:45
@jamesgardner1421
Copy link
Member Author

I can see that you are valiantly attempting to convert the positions to the correct units. Though I believe I did this already using the set_coordinates! function at the bottom of the file. The only thing that needed to be further converted was the unit cell before initalisation.

@clbox
Copy link

clbox commented Nov 23, 2022

I can see that you are valiantly attempting to convert the positions to the correct units. Though I believe I did this already using the set_coordinates! function at the bottom of the file. The only thing that needed to be further converted was the unit cell before initalisation.

ah ok thanks, so positions were right but I don't think the cell was converted originally.

Just trying stuff because I ran a benchmark and the enegy was factor ~1.7 out from running md_tian2 without the interface

@clbox
Copy link

clbox commented Nov 23, 2022

So now for a test structure:

NQCD + md_tian2: 0.5226285929371041 Ha.
MD_tian2 direct: 0.5226123679033052 Ha.

maybe close enough for now

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