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

FHI-aims IO Parsers #3435

Merged
merged 35 commits into from
Nov 8, 2023
Merged

FHI-aims IO Parsers #3435

merged 35 commits into from
Nov 8, 2023

Conversation

tpurcell90
Copy link
Contributor

@tpurcell90 tpurcell90 commented Oct 30, 2023

Summary

Creates an FHI-aims interface for pymatgen based off of the ASE parser and input generating structures. Part of this work was done in atomate2 originally for this PR (materialsproject/atomate2#562) where @ansobolev contributed.

This PR was made by the request of atomate2 so the IO can remain with pymatgen and not in that repoistory.

Major changes:

  • feature 1: IO objects for FHI-aims

Todos

  • Check how stress is stored as a property in structure
  • Confirm naming consistency
  • Convert doc strings from numpy format to Google format
  • Double check all type annotations are present

Checklist

  • Google format doc strings added. Check with ruff.
  • Type annotations included. Check with mypy.
  • Tests added for new features/fixes.
  • If applicable, new classes/functions/modules have 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:

pip install -U pre-commit
pre-commit install

1) parsers and AimsOuput ojbects moved over
2) All ASE Atoms objects removed in favor of pymatgen structures
3) TO DO: Check name schemes
4) Create an AimsInputs object in the future
1) remove aims_outputs naming convention
2) Add AimsGeometryIn and AimsControlIn classes
Adjusted from ASE parser tests
1) How to handle stress in properties?
For aims outputs checks
Test for both Si and H2O
Make sure that the AimsCubes are properly formatted
Add test for aims control.in file generators
as requested all output files are gzipped
1) Species blocks can now read from gzipped file
2) geometry.in can now read a gzip file
Error in the Actions from rounding errors, do more explicit test
for as_dict
1) Docstrings are googledoc format
2) All type hinting is done
Everything is also type hintted now
@tpurcell90
Copy link
Contributor Author

I think everything is now typehinted with google docstrings

@JaGeo
Copy link
Member

JaGeo commented Nov 2, 2023

@janosh I think this is ready to review as well

Forgot pip install in the last commit
@tpurcell90
Copy link
Contributor Author

Tests should now work

@janosh janosh added enhancement A new feature or improvement to an existing one io Input/output functionality labels Nov 5, 2023
@tpurcell90
Copy link
Contributor Author

I updated to master if those changes are needed for the docker images (errors are in tests I did not touch)

Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

Nice work @tpurcell90! 👍 🚀

Left a few comments.

tests/io/aims/test_aims_parsers.py Outdated Show resolved Hide resolved
tests/io/aims/test_aims_parsers.py Outdated Show resolved Hide resolved
tests/io/aims/test_aims_parsers.py Outdated Show resolved Hide resolved
tests/io/aims/test_aims_parsers.py Outdated Show resolved Hide resolved
tests/io/aims/test_aims_inputs.py Outdated Show resolved Hide resolved
1) Moved lines objects to seperate files
2) inline k_point_weights
3) Remove trailing comma
4) tmpdir -> tmp_path
@tpurcell90
Copy link
Contributor Author

Do we need to write a tutorial for the aims IO or are the docstrings good enough?

@tpurcell90
Copy link
Contributor Author

Also do I update the ChangeLog in a commit or is this done by the maintainers?

@janosh
Copy link
Member

janosh commented Nov 7, 2023

Do we need to write a tutorial for the aims IO or are the docstrings good enough?

You are very welcome to write a tutorial! 👍 Were you thinking a Jupyter notebook? I've been thinking about starting a /examples folder with tutorial notebooks in pymatgen.

Also do I update the ChangeLog in a commit or is this done by the maintainers?

We'll handle that.

pymatgen/io/aims/inputs.py Outdated Show resolved Hide resolved
pymatgen/io/aims/inputs.py Outdated Show resolved Hide resolved
pymatgen/io/aims/inputs.py Outdated Show resolved Hide resolved
pymatgen/io/aims/inputs.py Outdated Show resolved Hide resolved
use numpy eye instead of a full list

Co-authored-by: Janosh Riebesell <janosh.riebesell@gmail.com>
Signed-off-by: Thomas Purcell <tpurcell90@users.noreply.github.com>
@tpurcell90
Copy link
Contributor Author

Do we need to write a tutorial for the aims IO or are the docstrings good enough?

You are very welcome to write a tutorial! 👍 Were you thinking a Jupyter notebook? I've been thinking about starting a /examples folder with tutorial notebooks in pymatgen.

Also do I update the ChangeLog in a commit or is this done by the maintainers?

We'll handle that.

Ya I was thinking a basic jupyter notebook. I'll take a look at the examples tomorrow and make one up in the office (I am doing data transfer so I am not bringing my work computer home with me this week)

@tpurcell90
Copy link
Contributor Author

Do we need to write a tutorial for the aims IO or are the docstrings good enough?

You are very welcome to write a tutorial! 👍 Were you thinking a Jupyter notebook? I've been thinking about starting a /examples folder with tutorial notebooks in pymatgen.

Also do I update the ChangeLog in a commit or is this done by the maintainers?

We'll handle that.

Ya I was thinking a basic jupyter notebook. I'll take a look at the examples tomorrow and make one up in the office (I am doing data transfer so I am not bringing my work computer home with me this week)

I added an example file. I think everything is finished now

examples/aims_io/species_dir/14_Si_default.gz Outdated Show resolved Hide resolved
pymatgen/io/aims/inputs.py Outdated Show resolved Hide resolved
pymatgen/io/aims/inputs.py Outdated Show resolved Hide resolved
pymatgen/io/aims/inputs.py Outdated Show resolved Hide resolved
pymatgen/io/aims/inputs.py Outdated Show resolved Hide resolved
pymatgen/io/aims/inputs.py Show resolved Hide resolved
pymatgen/io/aims/inputs.py Outdated Show resolved Hide resolved
pymatgen/io/aims/inputs.py Outdated Show resolved Hide resolved
tests/io/aims/test_aims_inputs.py Outdated Show resolved Hide resolved
tpurcell90 and others added 9 commits November 8, 2023 20:14
1) Remove copy of species file in examples
2) Typo corrections in docstrings
3) d -> dct for `from_dict` methods
4) error mesages for AimsCube
5) Shortened default checks in Inputs
6) Removal of chdir in tests
…ut_files tests/io/aims/(aims_->'')parser_checks
Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

Thanks so much @tpurcell90! This was a big PR with nice docs and excellent test coverage, esp. for a 1st-time contributor! 👍

@janosh janosh enabled auto-merge (squash) November 8, 2023 19:40
@tpurcell90
Copy link
Contributor Author

No problem happy to add this into pymatgen! @janosh thanks for reviewing this!

aims.output -> aims.outputs module name
auto-merge was automatically disabled November 8, 2023 19:56

Head branch was pushed to by a user without write access

@tpurcell90
Copy link
Contributor Author

Corrected the module name in the test ref files, all tests should now pass. I forgot to do a final clean install of pymatgen when I made the names of the modules consistent.

@janosh janosh enabled auto-merge (squash) November 8, 2023 20:04
@janosh janosh merged commit d4e253d into materialsproject:master Nov 8, 2023
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement to an existing one io Input/output functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants