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.

Sign up for GitHub

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

Jump to bottom

DOS Fingerprints enhancements #3946

Merged
merged 18 commits into from
Aug 16, 2024
Merged

DOS Fingerprints enhancements #3946

merged 18 commits into from
Aug 16, 2024

Conversation

naik-aakash
Copy link
Contributor

@naik-aakash naik-aakash commented Jul 22, 2024 •
edited
Loading

Summary

  1. Add possibility to compare phonon dos , implementation is similar to electronic dos as implemented in Added methods to compute and compare DOS fingerprints #2772
  2. Add Wasserstein distance as metric besides existing Tanimoto index

Todo's

  • Add methods to compute fingerprints and compare phonon dos
  • Move methods to compute fingerprints and compare phonon dos to PhononDos class from CompletePhononDos class (Phonon workflow returns PhononDos objects in the taskdoc)
  • Update doc-strings where needed
  • Add tests for phonon dos comparisons
  • Adapt electronic dos fingerprint comparison to include Wasserstein metric
  • Add tests for updated electronic dos comparisons
  • Address review comments

@naik-aakash naik-aakash marked this pull request as draft July 22, 2024 15:10
@naik-aakash naik-aakash changed the title [WIP] DOS Fingerprints enhancements DOS Fingerprints enhancements Jul 29, 2024
@naik-aakash naik-aakash changed the title DOS Fingerprints enhancements [WIP] DOS Fingerprints enhancements Jul 29, 2024
@naik-aakash naik-aakash changed the title [WIP] DOS Fingerprints enhancements DOS Fingerprints enhancements Jul 29, 2024
@naik-aakash naik-aakash marked this pull request as ready for review July 29, 2024 15:01
@naik-aakash
Copy link
Contributor Author

Hi @shyuep , @janosh , @mkhorton and @JaGeo , this PR is ready for review.

JaGeo
JaGeo reviewed Jul 29, 2024
View reviewed changes
src/pymatgen/phonon/dos.py
Comment on lines +416 to +437
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)
"""
Copy link
Member

Choose a reason for hiding this comment

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

is this code very similar to the one for the electronic structure? if so, can one combine?

Copy link
Contributor Author

@naik-aakash naik-aakash Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Let's wait what the other reviewers think

Copy link
Member

Choose a reason for hiding this comment

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

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
janosh suggested changes Aug 1, 2024
View reviewed changes
Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

this looks very useful, thanks @naik-aakash! 👍
excited to start testing it in https://github.com/janosh/ffonons

src/pymatgen/electronic_structure/dos.py Outdated Show resolved Hide resolved
src/pymatgen/electronic_structure/dos.py Outdated Show resolved Hide resolved
src/pymatgen/phonon/dos.py
Comment on lines +416 to +437
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)
"""
Copy link
Member

Choose a reason for hiding this comment

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

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.

tests/phonon/test_dos.py Show resolved Hide resolved
@janosh janosh force-pushed the master branch 2 times, most recently from e3fbc67 to 41e6d99 Compare August 3, 2024 19:01
@naik-aakash
Copy link
Contributor Author

Thanks @janosh for the feedback. I will try to address the comments next week 😄

This was referenced Aug 12, 2024
Closed
Closed
@naik-aakash
Copy link
Contributor Author

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.

@janosh janosh merged commit 532281f into materialsproject:master Aug 16, 2024
33 checks passed
@janosh
Copy link
Member

janosh commented Aug 16, 2024

thanks @naik-aakash! 👍

@janosh janosh added enhancement A new feature or improvement to an existing one phonon Lattice vibrations labels Aug 16, 2024
@janosh janosh mentioned this pull request Aug 21, 2024
Open
@naik-aakash naik-aakash mentioned this pull request Sep 11, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JaGeo JaGeo JaGeo left review comments

@janosh janosh janosh requested changes

@shyuep shyuep Awaiting requested review from shyuep shyuep is a code owner

@mkhorton mkhorton Awaiting requested review from mkhorton mkhorton is a code owner

Assignees
No one assigned
Labels
enhancement A new feature or improvement to an existing one phonon Lattice vibrations
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@naik-aakash @janosh @JaGeo