materialsproject / atomate2

atomate2 is a library of computational materials science workflows
https://materialsproject.github.io/atomate2/
Other
156 stars 88 forks source link

Atomate2 jz pheasy_phonon #976

Open leslie-zheng opened 2 weeks ago

leslie-zheng commented 2 weeks ago

Summary

Include a summary of major changes in bullet points:

Additional dependencies introduced (if any)

TODO (if any)

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:

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.

JaGeo commented 2 weeks ago

@leslie-zheng Thanks 😃! I haven't looked at the code in detail yet. In any case, we would need tests and also install the pheasy code on the repo. The linting would need to pass as well.

Let me know if you need any advice for any of the points!

codecov[bot] commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 0% with 585 lines in your changes missing coverage. Please review.

Project coverage is 72.36%. Comparing base (8d57884) to head (93219ce). Report is 20 commits behind head on main.

Files with missing lines Patch % Lines
src/atomate2/common/schemas/pheasy.py 0.00% 305 Missing :warning:
src/atomate2/common/jobs/pheasy.py 0.00% 125 Missing :warning:
src/atomate2/common/flows/pheasy.py 0.00% 113 Missing :warning:
src/atomate2/vasp/flows/pheasy.py 0.00% 34 Missing :warning:
src/atomate2/vasp/jobs/pheasy.py 0.00% 8 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #976 +/- ## ========================================== - Coverage 75.70% 72.36% -3.34% ========================================== Files 147 161 +14 Lines 10925 12030 +1105 Branches 1613 1767 +154 ========================================== + Hits 8271 8706 +435 - Misses 2173 2813 +640 - Partials 481 511 +30 ``` | [Files with missing lines](https://app.codecov.io/gh/materialsproject/atomate2/pull/976?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=materialsproject) | Coverage Δ | | |---|---|---| | [src/atomate2/vasp/jobs/pheasy.py](https://app.codecov.io/gh/materialsproject/atomate2/pull/976?src=pr&el=tree&filepath=src%2Fatomate2%2Fvasp%2Fjobs%2Fpheasy.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=materialsproject#diff-c3JjL2F0b21hdGUyL3Zhc3Avam9icy9waGVhc3kucHk=) | `0.00% <0.00%> (ø)` | | | [src/atomate2/vasp/flows/pheasy.py](https://app.codecov.io/gh/materialsproject/atomate2/pull/976?src=pr&el=tree&filepath=src%2Fatomate2%2Fvasp%2Fflows%2Fpheasy.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=materialsproject#diff-c3JjL2F0b21hdGUyL3Zhc3AvZmxvd3MvcGhlYXN5LnB5) | `0.00% <0.00%> (ø)` | | | [src/atomate2/common/flows/pheasy.py](https://app.codecov.io/gh/materialsproject/atomate2/pull/976?src=pr&el=tree&filepath=src%2Fatomate2%2Fcommon%2Fflows%2Fpheasy.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=materialsproject#diff-c3JjL2F0b21hdGUyL2NvbW1vbi9mbG93cy9waGVhc3kucHk=) | `0.00% <0.00%> (ø)` | | | [src/atomate2/common/jobs/pheasy.py](https://app.codecov.io/gh/materialsproject/atomate2/pull/976?src=pr&el=tree&filepath=src%2Fatomate2%2Fcommon%2Fjobs%2Fpheasy.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=materialsproject#diff-c3JjL2F0b21hdGUyL2NvbW1vbi9qb2JzL3BoZWFzeS5weQ==) | `0.00% <0.00%> (ø)` | | | [src/atomate2/common/schemas/pheasy.py](https://app.codecov.io/gh/materialsproject/atomate2/pull/976?src=pr&el=tree&filepath=src%2Fatomate2%2Fcommon%2Fschemas%2Fpheasy.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=materialsproject#diff-c3JjL2F0b21hdGUyL2NvbW1vbi9zY2hlbWFzL3BoZWFzeS5weQ==) | `0.00% <0.00%> (ø)` | | ... and [11 files with indirect coverage changes](https://app.codecov.io/gh/materialsproject/atomate2/pull/976/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=materialsproject)
leslie-zheng commented 1 week ago

@leslie-zheng Thanks 😃! I haven't looked at the code in detail yet. In any case, we would need tests and also install the pheasy code on the repo. The linting would need to pass as well.

Let me know if you need any advice for any of the points!

Thanks Janine @JaGeo , I will make the linting check pass ASAP.

leslie-zheng commented 1 week ago

@JaGeo Hi Janine, will following all your suggestions and finish modifying it this weekend, thanks for the suggestions.

JaGeo commented 6 days ago

@leslie-zheng tgabk you! i think the goal should be that we reuse as much code as possible from the phonopy worfklow.

leslie-zheng commented 6 days ago

@leslie-zheng tgabk you! i think the goal should be that we reuse as much code as possible from the phonopy worfklow.

Hi Janine, Sure, I am working on it, already reuse the some repeated functions from phonopy section. After finish it again and will let you know.

leslie-zheng commented 3 days ago

@JaGeo Hi Janine, after finishing cleaning it up, and I test the whole workflow on our cluster. Take silicon as an example, it works well.

leslie-zheng commented 3 days ago

Hi @JaGeo , Maybe we can create a folder called phonon/ and move the /phonopy.py and /pheasy.py into? or a folder called lattice_dynamics/ instead.

JaGeo commented 3 days ago

@JaGeo Hi Janine, after finishing cleaning it up, and I test the whole workflow on our cluster. Take silicon as an example, it works well.

That's great. Could you add test workflows as well? There are many examples in the tests folder. Once this is done, I am happy to do further refactoring.

If you olan to add higher force constants as well, we can also keep the pheasy submodule for now.

leslie-zheng commented 1 day ago

@JaGeo Hi Janine, after finishing cleaning it up, and I test the whole workflow on our cluster. Take silicon as an example, it works well.

That's great. Could you add test workflows as well? There are many examples in the tests folder. Once this is done, I am happy to do further refactoring.

If you olan to add higher force constants as well, we can also keep the pheasy submodule for now.

Hi Janine @JaGeo , sure, will do it. yes, I am planning to add the higher-order force constants into.

leslie-zheng commented 1 day ago

Hi Janine @JaGeo ,

I finished adjusting the pheasy workflow based on previous linting checking again. And test the whole workflow again using silicon, working perfectly. For the pytest, I need help from you, I am not familiar with this process, cause I did not do it before. Could you please help me to write a example?, any documents from the pheasy workflow related to the pytest based on silicon case. I can packed it and shared with you.

best jiongzhi

JaGeo commented 1 day ago

@leslie-zheng yes, sure. Happy to help out with the tests