DOS Fingerprints enhancements#3946
Conversation
| def get_dos_fp( | ||
| self, | ||
| binning: bool = True, | ||
| min_f: float | None = None, | ||
| max_f: float | None = None, | ||
| n_bins: int = 256, | ||
| normalize: bool = True, | ||
| ) -> PhononDosFingerprint: | ||
| """Generate the DOS fingerprint. | ||
|
|
||
| Args: | ||
| binning (bool): If true, the DOS fingerprint is binned using np.linspace and n_bins. | ||
| Default is True. | ||
| min_f (float): The minimum mode frequency to include in the fingerprint (default is None) | ||
| max_f (float): The maximum mode frequency to include in the fingerprint (default is None) | ||
| n_bins (int): Number of bins to be used in the fingerprint (default is 256) | ||
| normalize (bool): If true, normalizes the area under fp to equal to 1. Default is True. | ||
|
|
||
| Returns: | ||
| PhononDosFingerprint: The phonon density of states fingerprint | ||
| of format (frequencies, densities, n_bins, bin_width) | ||
| """ |
There was a problem hiding this comment.
is this code very similar to the one for the electronic structure? if so, can one combine?
There was a problem hiding this comment.
Hi @JaGeo , yes it is similar, but we do not have option to get fingerprints for projected dos in case of phonons .
I think, If we want to combine both than we need to move these methods out of electronic and phonon dos class and possibly create another class just for getting fingerprints and comparisons which can accept PhononDos / Electronic dos objects, that can probably reside in pymatgen.analysis , maybe in fingerprints.py?
There was a problem hiding this comment.
Let's wait what the other reviewers think
There was a problem hiding this comment.
i'm easy. you know best how much redundancy there is and much how over-engineering (if any) it might involve to extract a common fingerprint.py module out of the e_dos and ph_dos modules.
janosh
left a comment
There was a problem hiding this comment.
this looks very useful, thanks @naik-aakash! 👍
excited to start testing it in https://github.com/janosh/ffonons
| def get_dos_fp( | ||
| self, | ||
| binning: bool = True, | ||
| min_f: float | None = None, | ||
| max_f: float | None = None, | ||
| n_bins: int = 256, | ||
| normalize: bool = True, | ||
| ) -> PhononDosFingerprint: | ||
| """Generate the DOS fingerprint. | ||
|
|
||
| Args: | ||
| binning (bool): If true, the DOS fingerprint is binned using np.linspace and n_bins. | ||
| Default is True. | ||
| min_f (float): The minimum mode frequency to include in the fingerprint (default is None) | ||
| max_f (float): The maximum mode frequency to include in the fingerprint (default is None) | ||
| n_bins (int): Number of bins to be used in the fingerprint (default is 256) | ||
| normalize (bool): If true, normalizes the area under fp to equal to 1. Default is True. | ||
|
|
||
| Returns: | ||
| PhononDosFingerprint: The phonon density of states fingerprint | ||
| of format (frequencies, densities, n_bins, bin_width) | ||
| """ |
There was a problem hiding this comment.
i'm easy. you know best how much redundancy there is and much how over-engineering (if any) it might involve to extract a common fingerprint.py module out of the e_dos and ph_dos modules.
e3fbc67 to
41e6d99
Compare
|
Thanks @janosh for the feedback. I will try to address the comments next week 😄 |
|
Hi @JaGeo and @janosh , I have now addressed review comments now and could be merged if there are no further comments. PS: Regarding creating a separate module for fingerprints, I had another detailed look at the code and concluded it would overcomplicate the code logic with minimal advantage of reducing few lines of code. So better to keep it in current place I feel as of now. |
|
thanks @naik-aakash! 👍 |
Summary
Wassersteindistance as metric besides existing Tanimoto indexTodo's
PhononDosclass fromCompletePhononDosclass (Phonon workflow returnsPhononDosobjects in the taskdoc)