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

Lattice dynamics workflow using Pheasy #1063

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

Conversation

hrushikesh-s
Copy link
Collaborator

Summary

Include a summary of major changes in bullet points:

  • Feature 1
  • Fix 1

Additional dependencies introduced (if any)

  • List all new dependencies needed and justify why. While adding dependencies that bring
    significantly useful functionality is perfectly fine, adding ones that add trivial
    functionality, e.g., to use one single easily implementable function, is frowned upon.
    Justify why that dependency is needed. Especially frowned upon are circular dependencies.

TODO (if any)

If this is a work-in-progress, write something about what else needs to be done.

  • Feature 1 supports A, but not B.

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.

Copy link

codecov bot commented Nov 22, 2024

Codecov Report

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

Project coverage is 3.95%. Comparing base (42bc7b8) to head (a8f805a).
Report is 19 commits behind head on main.

Files with missing lines Patch % Lines
src/atomate2/common/schemas/pheasy.py 0.00% 304 Missing ⚠️
src/atomate2/common/flows/pheasy.py 0.00% 125 Missing ⚠️
src/atomate2/common/jobs/pheasy.py 0.00% 115 Missing ⚠️
src/atomate2/vasp/flows/pheasy.py 0.00% 39 Missing ⚠️
src/atomate2/forcefields/flows/pheasy.py 0.00% 28 Missing ⚠️
src/atomate2/common/flows/phonons.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##            main   #1063      +/-   ##
========================================
- Coverage   4.32%   3.95%   -0.38%     
========================================
  Files        178     192      +14     
  Lines      12911   14246    +1335     
  Branches    1278    1421     +143     
========================================
+ Hits         559     564       +5     
- Misses     12321   13651    +1330     
  Partials      31      31              
Files with missing lines Coverage Δ
src/atomate2/common/flows/phonons.py 0.00% <0.00%> (ø)
src/atomate2/forcefields/flows/pheasy.py 0.00% <0.00%> (ø)
src/atomate2/vasp/flows/pheasy.py 0.00% <0.00%> (ø)
src/atomate2/common/jobs/pheasy.py 0.00% <0.00%> (ø)
src/atomate2/common/flows/pheasy.py 0.00% <0.00%> (ø)
src/atomate2/common/schemas/pheasy.py 0.00% <0.00%> (ø)

... and 9 files with indirect coverage changes

---- 🚨 Try these New Features:

@hrushikesh-s
Copy link
Collaborator Author

Pheasy workflow depends on Alamode. @janosh, can you pls help in installing Alamode via .github/workflows/testing.yml file?

@janosh
Copy link
Member

janosh commented Nov 22, 2024

no idea, never used or installed Alamode before. the best solution would be for them to publish pre-compiled wheels to PyPI so the code doesn't need to be re-compiled from scratch on every CI run as that's both slow and tends to be brittle

@JaGeo
Copy link
Member

JaGeo commented Nov 22, 2024

@naik-aakash set up a docker for https://github.com/autoatml/autoplex but it has been a massive amount of work. Before this, we did the compilation on the repo. The slowness was a real problem. But not sure it is realistic to get the wheels.
To sum up: compilation on the repo might be a realistic and okay solution if it isn't too slow.

@janosh
Copy link
Member

janosh commented Nov 22, 2024

Could offer an issue bounty to get buy in. Happy to pitch 50$

@JaGeo
Copy link
Member

JaGeo commented Nov 22, 2024

@janosh thank you for offering this.

I am unsure about the timescales for such solutions. We would like to have the workflow available as soom as possible due to the planned atomate2 paper.

@hrushikesh-s
Copy link
Collaborator Author

In that case, @JaGeo & @naik-aakash can you pls help with the compilation of alamode on the repo?
Also tagging in @leslie-zheng.

@naik-aakash set up a docker for https://github.com/autoatml/autoplex but it has been a massive amount of work. Before this, we did the compilation on the repo. The slowness was a real problem. But not sure it is realistic to get the wheels. To sum up: compilation on the repo might be a realistic and okay solution if it isn't too slow.

@hrushikesh-s
Copy link
Collaborator Author

These are the exact commands to install Alamode:

1. conda install -c conda-forge numpy scipy h5py compilers “libblas=*=*mkl” spglib boost eigen cmake ipython mkl-include
2. git clone https://github.com/ttadano/ALM.git
3. cd ALM
4. python setup.py build
5. pip install -e .

@JaGeo
Copy link
Member

JaGeo commented Nov 22, 2024

Just add it to the workflow file. You can see how micromamba is used for installation here:

micromamba install -n a2 -c conda-forge enumlib packmol bader openbabel openff-toolkit==0.16.2 openff-interchange==0.3.22 --yes

@@ -162,6 +162,7 @@ class BasePhononMaker(Maker, ABC):

kpath_scheme: str = "seekpath"
code: str = None
mp_id: str = None
Copy link
Member

Choose a reason for hiding this comment

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

This can be added as metadata.

@@ -202,6 +202,8 @@ def test_phonon_wf_vasp_only_displacements_no_structural_transformation(
assert_allclose(
responses[job.jobs[-1].uuid][1].output.primitive_matrix,
((0, 1, 0), (0, 0, 1), (1, 0, 0)),
rtol=1e-8,
Copy link
Member

Choose a reason for hiding this comment

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

@hrushikesh-s @naik-aakash and I are working on fixing these tests. I think I messed something up with the test files. I might have time later tonight. So, maybe skip those tests first.

Copy link
Member

Choose a reason for hiding this comment

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

Aah. Sorry! Different test 😅. But I meant the update to the latest phonopy. Sorry!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh okay okay. I was wondering why suddenly the test_phonons.py file was failing 😅

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.

4 participants