atomate2 icon indicating copy to clipboard operation
atomate2 copied to clipboard
verified publisher icon materialsproject

Addition of JDFTx code

Open cote3804 opened this issue 1 year ago • 8 comments

Summary

Adding support for JDFTx, an open source plane wave code that supports grand-canonical DFT and implements accurate solvation models.

Dependencies on other repositories

What we've done

  • Written JDFTx input and output parsers in pymatgen/io
  • Implemented base input set and generator plus three base input sets
  • Implemented basic calculation input and output schema
  • Written tests following CP2K and VASP format

Future Development

  • Make JDFTx compliant with the adsorbate workflows
  • Add more rich schemas and store custodian metadata in schemas

Developer Note

We are working with @mkhorton on this project. We are the developers behind the BEAST Database and have extensive experience using JDFTx for high-throughput workflows as well as direct collaboration with the developer of JDFTx.

cote3804 avatar Aug 13 '24 16:08 cote3804

Hi @cote3804, this is fantastic. Thank you for contributing it.

Firstly, would you be able to send me an email? My address is aganose [at] imperial [dot] ac [dot] uk.

For the PR, here are some high level comments to start with:

  • The best place for the IO functionality is pymatgen. Are you happy to submit a PR there once the input sets are completed? I'm happy for you to keep them in this PR while you work on finalising the workflows, as I realise this makes it easier for testing etc.
  • You can rename the emmet folders to schemas inline with atomate2 convention.
  • The READMEs and test files inside the atomate2/src directory aren't ideal. We can add a JDFTx page to the documentation (in docs) and the test files should go in tests (although currently it looks like most of these files will be in pymatgen anyway).
  • To help conform with the atomate2 code style, can you install pre-commit and run it on all of your files. This gets called automatically as part of the GitHub CI and the tests won't pass until all style issues are resolved.
pip install pre-commit
pre-commit run -all

Once you're ready for me to look at the input sets and jobs/flows please let me know (currently they have a lot of VASP code still).

utf avatar Aug 14 '24 08:08 utf

Hi @utf,

I emailed [email protected] but didn't get a reply. Is that the correct email? Your faculty profile seems to show it as [email protected].

cote3804 avatar Aug 16 '24 16:08 cote3804

Hi @utf

I think we're ready for you to look at the sets and jobs. You'll see that they're quite bare right now but functional. We haven't finished writing tests for the jobs, but @soge8904 is hard at work on that.

I tested the current scripts on NERSC's cluster and they are able to run JDFTx successfully starting from the set generators.

cote3804 avatar Sep 14 '24 20:09 cote3804

Hi everyone, I know this PR was previously blocked because it depended on materialsproject/pymatgen#4189 and materialsproject/pymatgen#4190. That dependency has now been merged, so I wanted to check if this PR can be reviewed again. I’ve updated the branch with the latest changes from main, made sure pre-commit checks pass, and verified that the tests run correctly. Let me know if anything else needs to be addressed!

soge8904 avatar Feb 18 '25 16:02 soge8904

Hi @utf This PR should now be ready to merge. All of the JDFTx related tests should pass. There is one cp2k test that's failing, however this is also the case for the main branch.

cote3804 avatar Apr 25 '25 12:04 cote3804

Hi @cote3804, apologies for the delay responding on this. Overall, I think it looks great. I'd just note that in most cases, we have now moved the input sets into pymatgen directly. Would you be OK to submit a PR to transition them over?

The only other thing is the jdftxcov.xml file which I think can be removed and added to the gitignore.

Once those changes are made I'm happy to merge it.

utf avatar Jun 19 '25 17:06 utf

Hi @utf

I'm happy to make both of those changes. Do you think it makes sense to merge the changes before moving the sets to pymatgen for the sake of having the JDFTx code working when the publication goes live? I'll still open the PR here and in pymatgen to move the sets.

cote3804 avatar Jun 19 '25 18:06 cote3804

Hi @utf,

Sorry for the long pause on this from my end. I deleted the jdftxcov.xml file and didn't include it in the gitignore because I don't think it's necessary.

I moved our base input set over to Pymatgen and tried to follow the VASP style as faithfully as I could. There was very little code I changed. The PR is active at https://github.com/materialsproject/pymatgen/pull/4479

cote3804 avatar Aug 13 '25 18:08 cote3804