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

Additional inputs to phonopy #1418

Merged
merged 4 commits into from
Jun 12, 2024
Merged

Additional inputs to phonopy #1418

merged 4 commits into from
Jun 12, 2024

Conversation

raynol-dsouza
Copy link
Contributor

  1. In order to calculate the projected DOS (either internally with phonopy or externally using a custom script), one needs the eigenvectors of the dynamical matrix at each (ideally irreducible) qpoint. This is by default hard-coded to be False in pyiron. I add an additional input to allow to save these eigenvectors if necessary.

  2. Phonopy has 2(?) methods for the integration over the Brillouin zone: the tetrahedron method (phonopy default, hard coded in pyiron) and the Gaussian smearing method (sigma calculated internally by phonopy). I also add an additional input to allow to change the default.

@raynol-dsouza raynol-dsouza requested a review from jan-janssen May 15, 2024 09:08
@coveralls
Copy link

coveralls commented May 15, 2024

Pull Request Test Coverage Report for Build 9094382462

Details

  • 2 of 4 (50.0%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.004%) to 71.059%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pyiron_atomistics/atomistics/master/phonopy.py 2 4 50.0%
Files with Coverage Reduction New Missed Lines %
pyiron_atomistics/atomistics/master/phonopy.py 1 45.5%
Totals Coverage Status
Change from base Build 9047055407: 0.004%
Covered Lines: 10629
Relevant Lines: 14958

💛 - Coveralls

@jan-janssen jan-janssen added format_black reformat the code using the black standard labels May 15, 2024
@raynol-dsouza
Copy link
Contributor Author

@jan-janssen Thanks for the format black commit!

Copy link
Contributor

@pmrv pmrv left a comment

Choose a reason for hiding this comment

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

If I remember correctly one can get the eigenvector also via some post processing from phonopy even from a loaded job, so it might not be necessary to save them and rather just compute them on demand (not sure how long this takes anymore). But that would be a bigger change and this is self contained and works, so looks good to me.

@raynol-dsouza
Copy link
Contributor Author

@pmrv yes, you are right. But this involves executing job.phonopy.run_mesh() again, which is basically repeating what could just be done once. It is also quite memory intensive to do it on demand depending on how dense the qpoint mesh is.

Copy link
Member

@jan-janssen jan-janssen left a comment

Choose a reason for hiding this comment

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

Looks good to me - please go ahead and merge the pull request so we include the changes in the next release.

@raynol-dsouza raynol-dsouza merged commit 88b456f into main Jun 12, 2024
25 checks passed
@raynol-dsouza raynol-dsouza deleted the phonopy_eigenvectors branch June 12, 2024 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
format_black reformat the code using the black standard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants