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

fix the calculation of fermi velocity #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yw-fang
Copy link

@yw-fang yw-fang commented Jul 26, 2019

Hi, developers

In the master branch, dk of 'get_fermi_velocities' function in analysis.py is defined as follows

                dk = np.sqrt((kpoints[i+1].cart_coords[0]
                              - kpoints[i].cart_coords[0])**2
                             + (kpoints[i+1].cart_coords[1]
                                - kpoints[i].cart_coords[1])**2)

However, kpoints are points in 3D k-space, hence they have the form of (kx, ky, kz). Hence, the distance between two points in k-space should be corrected as

                dk = np.sqrt((kpoints[i+1].cart_coords[0]
                              - kpoints[i].cart_coords[0])**2
                             + (kpoints[i+1].cart_coords[1]
                                - kpoints[i].cart_coords[1])**2
                             + (kpoints[i+1].cart_coords[2]
                                - kpoints[i].cart_coords[2])**2)

Please let me know if I miss something.
Thank you very much!

Yuewen

@ashtonmv
Copy link
Contributor

ashtonmv commented Oct 9, 2020

Hi @yw-fang, thanks for your contribution! get_fermi_velocities belongs to MPInterface's 2D materials submodule, where all kpoints are required to have the same reciprocal-z coordinate. So while it won't do any harm to add the delta z term like you did, it also won't make any difference for 2D materials.

I'm 50/50 about including it here, but this function should probably be moved into the main mpinterfaces module if non-2D users are making use of it. In that case it would definitely need to be included.

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.

None yet

2 participants