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

Including VASP surface adsorption flow #691

Merged
merged 154 commits into from
Sep 24, 2024

Conversation

itsduowang
Copy link
Contributor

@itsduowang itsduowang commented Jan 26, 2024

[This PR is still working in progress.]

Summary

including adsorption flow using VASP into atomate2

There are two files added into /vasp/flows and /vasp/jobs directories in order to implement surface adsorption calculation.

  • in /vasp/flows, the adsorption.py defines the general workflow of running the adsorption calculations for different surface-slab configurations and molecules.
  • in /vasp/jobs, the adsorption.py includes multiple elementary jobs for such workflow.

TODO

  • finish the rest of the code in atomate2/vasp/flows/adsorption.py and atomate2/vasp/jobs/adsorption.py
  • finish docstrings
  • test over more materials

Checklist

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 ruff and ruff format on your new code. This will
    automatically reformat your code to PEP8 conventions and fix many linting issues.
  • Doc strings 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 runpre-commit install and a check will be run prior to allowing commits.

itsduowang and others added 27 commits October 17, 2023 15:59
Copy link

codecov bot commented Mar 25, 2024

Codecov Report

Attention: Patch coverage is 93.78882% with 10 lines in your changes missing coverage. Please review.

Project coverage is 76.31%. Comparing base (8d57884) to head (5e0fa58).
Report is 32 commits behind head on main.

Files with missing lines Patch % Lines
src/atomate2/vasp/flows/adsorption.py 89.65% 3 Missing and 3 partials ⚠️
src/atomate2/vasp/jobs/adsorption.py 96.77% 2 Missing and 1 partial ⚠️
src/atomate2/vasp/schemas/adsorption.py 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #691      +/-   ##
==========================================
+ Coverage   75.70%   76.31%   +0.60%     
==========================================
  Files         147      159      +12     
  Lines       10925    11606     +681     
  Branches     1613     1719     +106     
==========================================
+ Hits         8271     8857     +586     
- Misses       2173     2234      +61     
- Partials      481      515      +34     
Files with missing lines Coverage Δ
src/atomate2/vasp/schemas/adsorption.py 90.00% <90.00%> (ø)
src/atomate2/vasp/jobs/adsorption.py 96.77% <96.77%> (ø)
src/atomate2/vasp/flows/adsorption.py 89.65% <89.65%> (ø)

... and 4 files with indirect coverage changes

@Zhuoying
Copy link
Contributor

Hi @itsduowang thanks for the PR. You can try to use pre-commit and ruff to fix any lint issues.

Install pre-commit hooks to auto-check types and linting before every commit

pip install -U pre-commit

pre-commit install

Use ruff to check and fix format

ruff format --check .

ruff check --fix

@tpurcell90
Copy link
Contributor

There would be interest in adding this workflow for FHI-aims as well. Do you have any recommendations on when we should port this for FHI-aims?

@itsduowang
Copy link
Contributor Author

Hi @tpurcell90, I am not familiar with the FHI-aims package. But I would love to work with you together to port this for FHI-aims!

@tpurcell90
Copy link
Contributor

Okay we'd be happy to work with you on that (FHI-aims is a nao basis set code). I think it would make sense for this one to get merged in first since it is already quite large. @utf do you agree with that it should wait for this to get merged in?

@itsduowang
Copy link
Contributor Author

Yes @tpurcell90, I think it would be better to merge the PR and then we can work on the adding FHI-aims support.

@utf, I have done the debugging for that K point issue and ran the pytest and testing workflow from my end (https://github.com/itsduowang/atomate2/actions/runs/10375913622). Please take a look and help me to rerun the testing. Thanks so much!

Copy link
Member

@utf utf left a comment

Choose a reason for hiding this comment

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

Thanks @itsduowang. Some more comments - this is almost there now. The main points are:

  • Better use of prev_dir.
  • Creating a schema for the adsorption data.

Let me know if you have any concerns about the comments.

src/atomate2/vasp/flows/adsorption.py Outdated Show resolved Hide resolved
src/atomate2/vasp/flows/adsorption.py Outdated Show resolved Hide resolved
src/atomate2/vasp/flows/adsorption.py Outdated Show resolved Hide resolved
src/atomate2/vasp/flows/adsorption.py Outdated Show resolved Hide resolved
src/atomate2/vasp/flows/adsorption.py Outdated Show resolved Hide resolved
src/atomate2/vasp/jobs/adsorption.py Outdated Show resolved Hide resolved
src/atomate2/vasp/jobs/adsorption.py Outdated Show resolved Hide resolved
src/atomate2/vasp/jobs/adsorption.py Outdated Show resolved Hide resolved
src/atomate2/vasp/jobs/adsorption.py Outdated Show resolved Hide resolved
src/atomate2/vasp/jobs/adsorption.py Outdated Show resolved Hide resolved
Copy link
Member

@utf utf left a comment

Choose a reason for hiding this comment

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

Edit: my comment got posted twice for some reason.

@itsduowang
Copy link
Contributor Author

Hi @utf, thanks for these updates. I will go through each item and resolve those issues.

@itsduowang
Copy link
Contributor Author

Hi @utf. I have resolved all the issues you mentioned above and included the schema into the adsorption workflow. Please take a look and let me know for any further issues or concerns.

@utf
Copy link
Member

utf commented Sep 24, 2024

Thanks @itsduowang. This looks great to me.

@utf utf enabled auto-merge (squash) September 24, 2024 14:38
@utf utf merged commit 2513903 into materialsproject:main Sep 24, 2024
6 checks passed
@utf utf added the feature A new feature being added label Sep 26, 2024
hrushikesh-s added a commit to hrushikesh-s/atomate2 that referenced this pull request Nov 16, 2024
* [WIP] adding surface adsorption calculation flow

* [WIP] adding surface adsorption calculation flow

* [WIP] adding surface adsorption calculation flow

* [WIP] adding surface adsorption calculation flow

* [WIP] adding surface adsorption calculation flow

* [WIP] adding surface adsorption calculation flow

* [WIP] adding surface adsorption calculation flow

* [wip] implementing more of jobs and flow in surface adsorption function

* [wip] implementing more of jobs and flow in surface adsorption function

* [wip] implementing more of jobs and flow in surface adsorption function

* [wip] implementing more of jobs and flow in surface adsorption function

* [wip] implementing more of jobs and flow in surface adsorption function

* [wip] implementing more of jobs and flow in surface adsorption function

* [wip] implementing more of jobs and flow in surface adsorption function

* [wip] implementing more of jobs and flow in surface adsorption function

* [wip] implementing more of jobs and flow in surface adsorption function

* temp test on push

* fix adsorption in flows and jobs

* add document in adsorption of jobs and flows

* check format

* [wip] implementing test for adsorption workflow

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] bug fixing

* [wip] updating VASP database

* fixes to the input structures

1. using the DFT relaxed structures for molecule & bulk slab instead of raw MP structures.

* Using a Molecule object instead of a Structure object for molecule_structure

* Add files via upload

* Delete tests/test_data/vasp/Au_adsorption/ads_relax_1_3/inputs directory

* Delete tests/test_data/vasp/Au_adsorption/ads_relax_2_3/inputs directory

* Delete tests/test_data/vasp/Au_adsorption/ads_relax_3_3/inputs directory

* Delete tests/test_data/vasp/Au_adsorption/bulk directory

* Delete tests/test_data/vasp/Au_adsorption/mol directory

* Add files via upload

* Add files via upload

* [wip] updating the test file

* [wip] updating the test file

* [wip] updating the test file

* [wip] updating the test file

* [wip] updating the test file

* [wip] updating the test file

* [wip] updating the test file

* [wip] updating the test file

* [wip] updating the test file

* [wip] updating the test file

* [wip] updating the test file

* [wip] updating the test file

* [wip] updating the test file

* [wip] updating the test_adsorption file

* [wip] updating the test_adsorption file

* [wip] updating the test_adsorption file

* [wip] updating the test_adsorption file

* fixing K point mesh

* fixing K point mesh

* fixing K point mesh

* Add files via upload

* Add files via upload

* Add files via upload

* Add files via upload

* Add files via upload

* Add files via upload

* Add files via upload

* Add files via upload

* Add files via upload

* Add files via upload

* Add files via upload

* Add files via upload

* Add files via upload

* Add files via upload

* Add files via upload

* Add files via upload

* Add files via upload

* Add files via upload

* Add files via upload

* fixing K point mesh

* fixing K point mesh

* fixing K point mesh

* fixing K point mesh

* fixing K point mesh

* fixing K point mesh

* fixing K point mesh

* fixing K point mesh

* fixing k points and creating a schema

* fixing k points and creating a schema

* creating a schema

* creating a schema

* creating a schema

---------

Co-authored-by: Xin_Chen <[email protected]>
Co-authored-by: Xin Chen <[email protected]>
Co-authored-by: Hrushikesh Sahasrabuddhe <[email protected]>
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.

7 participants