-
Notifications
You must be signed in to change notification settings - Fork 108
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
Add JDFTx jobs.py to run JDFTx using atomate2/jobflow #349
base: master
Are you sure you want to change the base?
Conversation
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.
Added some comments.
Also missing are unit tests for JDFTx (see "tests" module). Don't think this can be merged in without tests.
src/custodian/jdftx/jobs.py
Outdated
stderr_file="std_err.txt", | ||
) -> None: | ||
""" | ||
This constructor is necessarily complex due to the need for |
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 think you can remove the note about the constructor being "complex", it doesn't seem to be complex. I believe this note is copy-pasted from another constructor that is complex
src/custodian/jdftx/jobs.py
Outdated
): | ||
# use line buffering for stderr | ||
return subprocess.run( | ||
cmd.split(), |
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.
should prefer shlex.split()
src/custodian/jdftx/jobs.py
Outdated
for cmd in self.jdftx_cmd: | ||
if "jdftx" in cmd: | ||
try: | ||
os.system(f"killall {cmd}") |
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.
See implementation of terminate() in VASP job to avoid killing all JDFTx processes when possible
… conftest.py to store fixtures
Merge into main
Summary
Hi,
This PR is to add jobs.py for JDFTx. We have a draft PR open on atomate2 to integrate JDFTx, but it seems that this script belongs here. Jobs.py was created using the CP2K template, with only the basic functionalities for now (just enough to run a job).
Todos
If this is work in progress, what else needs to be done?
Checklist
ruff
.mypy
.duecredit
@due.dcite
decorators to reference relevant papers by DOI (example)Tip: Install
pre-commit
hooks to auto-check types and linting before every commit: