-
Notifications
You must be signed in to change notification settings - Fork 5
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
DFTB+ interface upgrade #179
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #179 +/- ##
=======================================
Coverage 91.86% 91.86%
=======================================
Files 47 47
Lines 6748 6748
Branches 758 758
=======================================
Hits 6199 6199
Misses 537 537
Partials 12 12
Flags with carried forward coverage won't be shown. Click here to find out more. |
d47720a
to
59a915b
Compare
59a915b
to
984c710
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not tested the code on my machine. I added general comments, mainly on what to do when people do not work on clusters with the same env as in Prague. Also, might be worth adding a README file for DFTB or at least add somewhere link for documentation and for different parameters.
interfaces/DFTB/dftb_in.hsd
Outdated
charge = 0.0 | ||
#ReadInitialCharges = yes | ||
SlaterKosterFiles = Type2FileNames { | ||
Prefix = "/home/hollas/3ob-2-1/" | ||
Prefix = "/home/hollas/3ob-3-1/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably add something general with a comment for users, instead of hollas home which will not be present on most clusters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this and added a note at the top of r.dftb
that this file needs to be modified.
input=input$ibead | ||
geom=../geom.dat.$ibead | ||
if [[ -f ../SetEnvironment.sh ]]; then | ||
# This is specific to Prague clusters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should people do if they do not use the Prague clusters? Remove SetEnvironment or change it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If they are not on Prague clusters this file will simply not exist.
interfaces/DFTB/r.dftb
Outdated
# This is specific to Prague clusters | ||
source ../SetEnvironment.sh DFTB | ||
else | ||
# We assume dftb+ is in PATH already |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some test would be better instead of assumption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opted not to add a check for this, since if it is not in the PATH we will fail to execute anyway, and the default unix error message should be clear enough. I did add a note about this at the top of the file though.
I expanded the comment at the top of the |
geom=../geom.dat."$ibead" | ||
natom=$(wc -l < "$geom") | ||
|
||
function prepare_dftb_inputs() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created two new functions, generate_dftb_inputs
and extract_energy_and_gradients
, hopefully this makes the high-level flow of the script easier to follow. Happy to hear suggestions how to make this even better.
I've tested the refactored interface locally with DFTB+ 24.1 by checking the energy conservation in a short NVE simulation of a single water molecule
So this is now ready for review again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My questions were answered and the new implemented functions look good!
No description provided.