-
Notifications
You must be signed in to change notification settings - Fork 868
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
Added methods to compute and compare DOS fingerprints #2772
Added methods to compute and compare DOS fingerprints #2772
Conversation
…into dos_fingerprinting
for more information, see https://pre-commit.ci
…into dos_fingerprinting git pull
pymatgen/electronic_structure/dos.py
Outdated
"some error in the spelling." | ||
) | ||
|
||
def _fp_to_dict(self, fp): |
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.
It could be static, I assume
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.
Yes, It could be. Will make the changes. Thanks for the feedback 😃
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.
You could replace one "finger print" with fingerprint in the documentation but otherwise it looks fine from my side.
Thanks for the review @JaGeo 😄 |
Hi @janosh , This PR is ready to be merge. |
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.
Thanks @naik-aakash! I left some comments.
pymatgen/electronic_structure/dos.py
Outdated
@@ -1165,6 +1166,150 @@ def get_upper_band_edge( | |||
upper_band_edge = energies[np.argmax(densities)] | |||
return upper_band_edge | |||
|
|||
def get_dos_fp(self, type="summed_pdos", binning=True, min_e=None, max_e=None, nbins=256, normalize=True): |
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.
Would be good to add type annotations here.
def get_dos_fp(self, type: str = "summed_pdos", binning: bool = True, ...
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.
Thanks for the suggestions , I have made the requested changes.
pymatgen/electronic_structure/dos.py
Outdated
type (str): Specify fingerprint type needed can accept 's/p/d/f/summed_pdos/tdos' | ||
min_e (float): The minimum mode energy to include in the fingerprint | ||
max_e (float): The maximum mode energy to include in the fingerprint | ||
nbins (int): Number of bins to be used in the fingerprint |
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 we rename this to n_bins
for proper snake casing?
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.
Done 😃
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Remove unnecessary file
Hi @janosh , all suggestions have been incorporated now. And tests seems to pass as well. 😄 |
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.
@naik-aakash Thanks, looks great! Just a few more nitpicks.
Hi @janosh , thank you for the really useful tips and suggestions. 😃 |
Hi @janosh , I do not know why so many tests are failing. I looked at Failed tests and they do not have anything to do with this PR. I hope it could still be merged. |
@naik-aakash Sorry for the delay here. Back from traveling now. PR looks good! Made a few last changes. Failures like you said are unrelated. Ready to merge. |
@janosh No worries! Thank you. Happy Holidays |
…ct#2772) * added dos_fingerprint and similarity index methods * Added test cases and reformatted and cleaned code * added binwidth ,renamed states to densities in fp obj,updated tests * added source link * changed get_dos_fp_similarity and fp_to_dict methods to static * Delete Test.py, unnecessary file * simplified dict updating, added missing type annotations * NamedTuple return type fixed * small clean up * document get_dos_fp() and get_dos_fp_similarity() raise conditions in doc str * add types for fp1,fp2 and update doc str * update exception tests Co-authored-by: anaik <anaik@sv2218.zit.bam.de> Co-authored-by: Janosh Riebesell <janosh.riebesell@gmail.com>
…ct#2772) * added dos_fingerprint and similarity index methods * Added test cases and reformatted and cleaned code * added binwidth ,renamed states to densities in fp obj,updated tests * added source link * changed get_dos_fp_similarity and fp_to_dict methods to static * Delete Test.py, unnecessary file * simplified dict updating, added missing type annotations * NamedTuple return type fixed * small clean up * document get_dos_fp() and get_dos_fp_similarity() raise conditions in doc str * add types for fp1,fp2 and update doc str * update exception tests Co-authored-by: anaik <anaik@sv2218.zit.bam.de> Co-authored-by: Janosh Riebesell <janosh.riebesell@gmail.com>
…ct#2772) * added dos_fingerprint and similarity index methods * Added test cases and reformatted and cleaned code * added binwidth ,renamed states to densities in fp obj,updated tests * added source link * changed get_dos_fp_similarity and fp_to_dict methods to static * Delete Test.py, unnecessary file * simplified dict updating, added missing type annotations * NamedTuple return type fixed * small clean up * document get_dos_fp() and get_dos_fp_similarity() raise conditions in doc str * add types for fp1,fp2 and update doc str * update exception tests Co-authored-by: anaik <anaik@sv2218.zit.bam.de> Co-authored-by: Janosh Riebesell <janosh.riebesell@gmail.com>
* added dos_fingerprint and similarity index methods * Added test cases and reformatted and cleaned code * added binwidth ,renamed states to densities in fp obj,updated tests * added source link * changed get_dos_fp_similarity and fp_to_dict methods to static * Delete Test.py, unnecessary file * simplified dict updating, added missing type annotations * NamedTuple return type fixed * small clean up * document get_dos_fp() and get_dos_fp_similarity() raise conditions in doc str * add types for fp1,fp2 and update doc str * update exception tests Co-authored-by: anaik <anaik@sv2218.zit.bam.de> Co-authored-by: Janosh Riebesell <janosh.riebesell@gmail.com>
Compute and compare fingerprints of project and total density of states.
This functionality could be really useful to compare the DOS of materials from different softwares (eg:- LOBSTER and VASP ) or versions (Eg:- VASP v5 and VASP v6)