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

Add FHI-aims DFT calculator #562

Merged
merged 127 commits into from
Feb 29, 2024
Merged

Conversation

tpurcell90
Copy link
Contributor

@tpurcell90 tpurcell90 commented Oct 11, 2023

Summary

Create an FHI-aims interface into atomate2 (moved from: https://github.com/tpurcell90/atomate2-fhi-aims). This was originally started as a project to incorporate FHI-aims into jobflow as a part of the NOMAD-CoE, working with @davidwaroquiers and @ansobolev. Originally we did not know if this could be incorporated into atomate2, which is why we had a separate repository, but everyone now agrees that we should move it to atomate2.

  • FHI-aims interface for base jobs, input generators and schemas
  • ASE interface that is aims specific (can be moved into a general framework if everyone agrees)
  • Generalization of the phonon workflow originally made for VASP

Additional dependencies introduced (if any)

  • ASE: This is needed because currently the only way to interact with FHI-aims in python is through ASE. We are in the process of discussing a new FHI-aims controlled python interface, but I have not gotten any type of general support to start to write that. Additionally the SocketIO calculators are useful for the phonon calculations as it can speed up the SCF convergence in FHI-aims by a factor of ~2. The largest problem with this however, is the delay in getting ASE 3.23 out, which is needed for this code interface
  • seekpath: This is used in the common phonon workflows, but I did not see it in the dependencies

Note I did not add these to the pyproject file before discussing

TODO (if any)

  • Discussion of ASE dependency
  • @JaGeo can you check that my modifications to the phonon workflow did not break the VASP workflow? I don't have a VASP license to check this
  • run ruff
  • run mypy

Checklist

Work-in-progress pull requests are encouraged, but please put [WIP] in the pull request
title.

Before a pull request can be merged, the following items must be checked:

  • Code is in the standard Python style.
    The easiest way to handle this is to run the following in the correct sequence on
    your local machine. Start with running black on your new code. This will
    automatically reformat your code to PEP8 conventions and removes most issues. Then run
    ruff.
  • Docstrings have been added in the Numpy docstring format.
    Run ruff on your code.
  • Type annotations are highly encouraged. Run mypy to
    type check your code.
  • Tests have been added for any new functionality or bug fixes.
  • All linting and tests pass.

Note that the CI system will run all the above checks. But it will be much more
efficient if you already fix most errors prior to submitting the PR. It is highly
recommended that you use the pre-commit hook provided in the repository. Simply run
pre-commit install and a check will be run prior to allowing commits.

This was originally started as a project to incorporate fhi-aims into
jobflow as a part of the NOMAD-CoE, working with @davidwaroquiers and
@ansobolev. Originally we did not know if this could be incorporated into
atomate2, which is why we had a seperate repository, but the structure and
administrative approval were recently granted.
Minor things came up from the migration:
1) Remove all atomate2_temp from out.json files
2) Compare files function now in tests.aims not tests
3) Make all socket tests skippable because they require
   an aims binary to run
imports should now be fixed
@JaGeo
Copy link
Member

JaGeo commented Oct 11, 2023

@tpurcell90 @utf I am happy to help. I left one immediate comment there already. For the rest, I need more time.

@utf
Copy link
Member

utf commented Oct 11, 2023

Thanks for the PR. In general this looks really high quality and hopefully there won't be much of a barrier to getting it merged. From a quick scan through, it looks like merging the phonon workflows will be the main challenge. If you're happy to coordinate that aspect @JaGeo that would be great.

In terms of getting the linting to pass, I'd recommend installing pre-commit, with:

pip install pre-commit
pre-commit install  # run inside atomate2 folder

This will automatically run the linting every time you do a commit. To run on the existing files, you can do:

pre-commit run --all

@JaGeo
Copy link
Member

JaGeo commented Oct 11, 2023

  • seekpath: This is used in the common phonon workflows, but I did not see it in the dependencies

We have additional dependencies for the phonon runs. It is part of it.

Copy link
Member

@JaGeo JaGeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tpurcell90 I was too curious to not look at it.
I left several comments. One major comment that I have: The phonon forcefield workflow should probably also inherit from this new base class if possible: https://github.com/materialsproject/atomate2/blob/main/src/atomate2/forcefields/flows/phonons.py

src/atomate2/vasp/flows/phonons.py Outdated Show resolved Hide resolved
src/atomate2/vasp/flows/phonons.py Outdated Show resolved Hide resolved
src/atomate2/vasp/flows/phonons.py Outdated Show resolved Hide resolved
src/atomate2/common/flows/phonons.py Outdated Show resolved Hide resolved
src/atomate2/common/flows/phonons.py Outdated Show resolved Hide resolved
src/atomate2/common/jobs/phonons.py Outdated Show resolved Hide resolved
src/atomate2/common/jobs/phonons.py Outdated Show resolved Hide resolved
src/atomate2/common/jobs/phonons.py Outdated Show resolved Hide resolved
src/atomate2/common/schemas/phonons.py Outdated Show resolved Hide resolved
src/atomate2/common/schemas/phonons.py Outdated Show resolved Hide resolved
tpurcell90 and others added 10 commits October 11, 2023 15:08
Fix all of the linter errors I missed using flake8 over
ruff and mpypy

Also fixed spelling errors
Fixes python3.9 typing error as X | Y is for python3.10
Co-authored-by: J. George <JaGeo@users.noreply.github.com>
Left to do:
1) ForceField phonon flow
2) overall documentation checks
Everything should be up to date now
Checked the diff between this an main and they should now be consistent
1) Missed one Optional typing
2) I set the factor for forcefields to the same as VASP as that was
the code that was passed to the schema earlier
Uses the latest gitlab version as we wait for the new release
pyproject.toml Outdated Show resolved Hide resolved
@tpurcell90
Copy link
Contributor Author

@tpurcell90 I was too curious to not look at it. I left several comments. One major comment that I have: The phonon forcefield workflow should probably also inherit from this new base class if possible: https://github.com/materialsproject/atomate2/blob/main/src/atomate2/forcefields/flows/phonons.py

The forcefields now have the same format as VASP/aims

@JaGeo
Copy link
Member

JaGeo commented Oct 11, 2023

The forcefields now have the same format as VASP/aims

@tpurcell90 Awesome! Thank you.

@JaGeo
Copy link
Member

JaGeo commented Oct 12, 2023

@utf prev_dir is now breaking the VASP workflow. Do you maybe have a good suggestion to solve this? Should we define prev_dir in addition to prev_vasp_dir or rename completely?

@tpurcell90
Copy link
Contributor Author

Another option would be to add both now and give a deprecation warning for prev_vasp_dir to be removed at a later date.

@utf
Copy link
Member

utf commented Oct 12, 2023

For now, can we follow the approach taken in the Magnetic ordering maker (see here) from this PR: #432

I will make a separate issue about renaming prev_vasp_dir which we can then implement after some consultation.

1) conftest for aims checks if addoption is possible, and if not then always mocks aims
2) parsers constraint tests were incorrectly modified during fixing linting errors
3) prev_dir host split is now working correctly in convergence tests
4) Correct pydantic errors in AimsOutput
5) Aims files error fix
Following @utf's suggestion to match the magnetisim workflow
This works for aims, but VASP and forcefields I have enviornment errors
pushing to test
The output should only be converted if it is a MSONableAtoms object
not a Structure object
@tpurcell90
Copy link
Contributor Author

tpurcell90 commented Oct 12, 2023

I used that approach for the phonon workflows as well. I set the forcefields' argname to None since it does not look like that used a prev_dir keyword at all

@tpurcell90
Copy link
Contributor Author

AimsInputSet is now in pymatgen master. So I need to wait for the next version to be released

@JaGeo
Copy link
Member

JaGeo commented Feb 8, 2024

@tpurcell90 You might likely run into this error. I added some hints here: https://github.com/materialsproject/atomate2/pull/693/files

@tpurcell90
Copy link
Contributor Author

Thanks I'll take a look at those once I am back from class!

@tpurcell90
Copy link
Contributor Author

So looking at this the pymatgen change is also in #693 so I think it makes more sense to wait for that to go in then deal with the merge conflicts of the two fixes.

@utf utf mentioned this pull request Feb 15, 2024
13 tasks
@utf
Copy link
Member

utf commented Feb 28, 2024

Hi @tpurcell90, are you able to resolve the remaining conflicts? Then this should be be mergeable.

tpurcell90 and others added 3 commits February 28, 2024 07:48
Forgot to check when resolving conflicts online
should fix remaining conflicts
@tpurcell90
Copy link
Contributor Author

@utf This is now ready, let me know if there are any more final changes you want me to make

@utf
Copy link
Member

utf commented Feb 29, 2024

Fantastic, thank you @ab5424 and @tpurcell90!

@utf utf merged commit 42742ce into materialsproject:main Feb 29, 2024
6 checks passed
@utf utf added the feature A new feature being added label Feb 29, 2024
@utf utf changed the title atomate2 aims Add FHI-aims DFT calculator Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature being added
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants