-
Notifications
You must be signed in to change notification settings - Fork 89
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
Phonons for forcefields #398
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #398 +/- ##
==========================================
+ Coverage 65.32% 65.48% +0.16%
==========================================
Files 74 78 +4
Lines 7189 7437 +248
Branches 930 961 +31
==========================================
+ Hits 4696 4870 +174
- Misses 2209 2267 +58
- Partials 284 300 +16
|
@gpetretto FYI. @QuantumChemist and I will work on this implemention in the next 1-2 months. |
Closes #409 as well. |
I have added the force constants to the data store as well. [x] Tests for the workflow |
@utf and maybe also @gpetretto: maybe you have some thoughts with regard to generalization. As mentioned above, BORN charges and potentially also relying on previous DFT computations are an issue for generalization of the workflow. E.g., this line: https://github.com/JaGeo/atomate2/blob/ae043eb0d321ad630e4593dd68bc6a0042ebbe6e/src/atomate2/vasp/flows/phonons.py#L314 or this line: |
8f54d31
to
a42608a
Compare
I will potentially finish the workflow including tests for the forcefield part for now. Then, we really need to do a discussion on how "common" workflows where several DFT/force field runs are included should be generalized. I could define abstract methods for born charge computation, bulk relaxation, static runs that we than overwrite for each specific code. "prev_vasp_dir" is one of the issues that require this. Or we could come up with a general way of connecting jobs ("prev_dir"). This would then, however, require a lot of refactoring and also a lot of thinking. |
I adapted my work plan a bit. Some parts could also be included within another PR. |
…orcefield-phonons
…nto forcefield-phonons
Forcefield phonons
Forcefield phonons
Phononforcefield
…orcefield-phonons
adjusted some small things in forcefield PhononMaker
Thanks very much for this @JaGeo. I think we can leave the discussion for merging the vasp and forcefield phonon workflows to a separate PR. I have some ideas for this which I can raise in an issue. I put a couple of very minor comments on the PR. Can you also rename |
Thanks. |
src/atomate2/common/jobs/phonons.py
Outdated
phonon_jobs.append(phonon_job) | ||
outputs["displacement_number"].append(i) | ||
outputs["uuids"].append(phonon_job.output.uuid) | ||
outputs["dirs"].append(phonon_job.output.dir_name) | ||
outputs["forces"].append(phonon_job.output.output.forces) | ||
outputs["displaced_structures"].append(displacement) | ||
# outputs["displaced_structures"].append(displacement) |
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.
Remove?
src/atomate2/common/jobs/phonons.py
Outdated
@@ -279,7 +284,7 @@ def run_phonon_displacements( | |||
"forces": [], | |||
"uuids": [], | |||
"dirs": [], | |||
"displaced_structures": [], | |||
# "displaced_structures": [], |
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.
Remove?
@utf , merge conflicts and your suggestions should be addressed! I hope it is ready to merge! |
Fantastic. Thanks very much! |
Summary
Closes #371 (in the future). It's a work in progress.
Plans:
This is a first output for Si of the workflow with CHGNet:
We will also validate the results carefully.