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

Atomate2 jz pheasy_phonon #976

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

leslie-zheng
Copy link

@leslie-zheng leslie-zheng commented Sep 13, 2024

Summary

Include a summary of major changes in bullet points:

  • Using Pheasy code (LASSO) to efficiently extract force constants for phonon calculation
  • Based on this phonon workflow, a relative small lattice constants (a,b and c axes) can be used to accurately extract second order force constants (usually 12 Angstrum is enough for most of materials)
  • Fix 1

Additional dependencies introduced (if any)

TODO (if any)

  • Quality check for all phonon calculations,
  • Extracting Higher-order force constants, including up to six order.
  • Anharmonic phonon renormalization using Modified SSCHA combing the MC method, relaxing atomic site coordinates and bubble self-energy.
  • Lattice thermal conductivity calculation incorporating diagonal and non-diagonal terms of heat flux operators.

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 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 run
pre-commit install and a check will be run prior to allowing commits.

@JaGeo
Copy link
Member

JaGeo commented Sep 13, 2024

@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!

Copy link

codecov bot commented Sep 13, 2024

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 ⚠️
src/atomate2/common/jobs/pheasy.py 0.00% 125 Missing ⚠️
src/atomate2/common/flows/pheasy.py 0.00% 113 Missing ⚠️
src/atomate2/vasp/flows/pheasy.py 0.00% 34 Missing ⚠️
src/atomate2/vasp/jobs/pheasy.py 0.00% 8 Missing ⚠️
Additional details and impacted files
@@            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 Coverage Δ
src/atomate2/vasp/jobs/pheasy.py 0.00% <0.00%> (ø)
src/atomate2/vasp/flows/pheasy.py 0.00% <0.00%> (ø)
src/atomate2/common/flows/pheasy.py 0.00% <0.00%> (ø)
src/atomate2/common/jobs/pheasy.py 0.00% <0.00%> (ø)
src/atomate2/common/schemas/pheasy.py 0.00% <0.00%> (ø)

... and 11 files with indirect coverage changes

@leslie-zheng
Copy link
Author

@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.

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.

I have left some further comments on code structure. It would be good to change them.

Additional points:

  • automated tests would be needed
  • automatic installation of pheasy and the other codes

I am happy to discuss this in further detail or help out!

src/atomate2/common/flows/pheasy.py Outdated Show resolved Hide resolved
src/atomate2/common/flows/pheasy.py Outdated Show resolved Hide resolved
src/atomate2/common/flows/pheasy.py Outdated Show resolved Hide resolved
src/atomate2/common/flows/pheasy.py Outdated Show resolved Hide resolved
src/atomate2/common/flows/pheasy.py Outdated Show resolved Hide resolved
src/atomate2/common/jobs/pheasy.py Outdated Show resolved Hide resolved
src/atomate2/common/jobs/pheasy.py Show resolved Hide resolved
src/atomate2/common/schemas/pheasy.py Outdated Show resolved Hide resolved
src/atomate2/common/schemas/pheasy.py Show resolved Hide resolved
src/atomate2/vasp/jobs/pheasy.py Outdated Show resolved Hide resolved
@leslie-zheng
Copy link
Author

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

@JaGeo
Copy link
Member

JaGeo commented Sep 23, 2024

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

@leslie-zheng
Copy link
Author

@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.

Copy link
Author

@leslie-zheng leslie-zheng left a comment

Choose a reason for hiding this comment

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

There seem to be more parameters here. Maybe, you can check the whole code again for such points. I might overlook parts here.

Hi Janine, Thanks for the comments.
Sure, will do. I am cleaning up the code these days. When I finished and let you know.
By the way, The pheasy branch is not only focusing on LASSO-based phonon calculations, in the near future, we also integrate the higher-order force constant calculation and phonon renormalization functions into.

with open("force_matrix.pkl","wb") as file:
pickle.dump(set_of_forces_a,file)
pickle.dump(dataset_forces_array_disp,file)
Copy link
Member

Choose a reason for hiding this comment

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

Can you only use pickle here?

Copy link
Author

Choose a reason for hiding this comment

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

Can you only use pickle here?

other format also should be fine, any suggestions? because in pheasy, it uses the pickle, so i just simply follow it.

@leslie-zheng
Copy link
Author

@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
Copy link
Author

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
Copy link
Member

JaGeo commented Sep 27, 2024

@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
Copy link
Author

@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
Copy link
Author

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
Copy link
Member

JaGeo commented Sep 29, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants