-
Notifications
You must be signed in to change notification settings - Fork 101
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
base: main
Are you sure you want to change the base?
Atomate2 jz pheasy_phonon #976
Conversation
@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 ReportAttention: Patch coverage is
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
|
Thanks Janine @JaGeo , I will make the linting check pass ASAP. |
There was a problem hiding this 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!
@JaGeo Hi Janine, will following all your suggestions and finish modifying it this weekend, thanks for the suggestions. |
Modified some parts based on Janine's comments.
…atomate2 into atomate2_jz_pheasy
@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. |
There was a problem hiding this 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.
…group for crystals.
with open("force_matrix.pkl","wb") as file: | ||
pickle.dump(set_of_forces_a,file) | ||
pickle.dump(dataset_forces_array_disp,file) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@JaGeo Hi Janine, after finishing cleaning it up, and I test the whole workflow on our cluster. |
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. |
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. |
Hi Janine @JaGeo , I finished adjusting the pheasy workflow based on previous linting checking again. best |
@leslie-zheng yes, sure. Happy to help out with the tests |
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:
The easiest way to handle this is to run the following in the correct sequence on
your local machine. Start with running
ruff
andruff format
on your new code. This willautomatically reformat your code to PEP8 conventions and fix many linting issues.
Run ruff on your code.
type check your code.
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.