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

Minor update to make FermiDos more robust #4240

Merged

Conversation

kavanase
Copy link
Contributor

@kavanase kavanase commented Dec 29, 2024

This is a minor fix to the get_doping method in FermiDos to make it a little more robust, so that it can handle rare cases where there is only either 0 or 1 energy increments between the VBM and CBM indices (e.g. Dirac cone systems or toy test cases). This is done by starting the CB integral (for electron concentrations) from either the mid-gap index or, if mid-gap index is the VBM, from VBM+1 (= CBM index), and vice versa for the VB integral for hole concentrations.

@kavanase kavanase force-pushed the minor_FermiDos_robustness_update branch from db71bbe to d1f986d Compare December 29, 2024 14:05
kavanase added a commit to SMTG-Bham/doped that referenced this pull request Dec 30, 2024
@shyuep
Copy link
Member

shyuep commented Dec 31, 2024

Thanks. Can you add a unittest for the functionality pls?

@kavanase
Copy link
Contributor Author

kavanase commented Jan 3, 2025

Done! Didn't add one at first as it should be a relatively rare case where this actually matters (e.g. Dirac cone systems or toy test cases), but added now!
Some tests seem to be failing for unrelated reasons

@mkhorton
Copy link
Member

mkhorton commented Jan 9, 2025

Are you sure they're unrelated? I see e.g.

    def test_get_gap(self):
>       assert self.dos.get_gap() == approx(2.0589, abs=1e-4)
E       assert np.float64(2.3163) == 2.0589 � 1.0e-04
E         
E         comparison failed
E         Obtained: 2.3163
E         Expected: 2.0589 � 1.0e-04

Thanks for the PR!

@mkhorton mkhorton added the awaiting user Needs more information from OP. label Jan 9, 2025
@kavanase
Copy link
Contributor Author

@mkhorton ah yes sorry! I just looked at one failing test and it seemed to be something different.

I've fixed that one now, and it's a nice validation as it now gives a Dos.get_gap() value which better matches the eigenvalue_band_properties result (which is typically the most reliable).

@shyuep shyuep enabled auto-merge (squash) January 17, 2025 17:26
@shyuep shyuep merged commit e77a496 into materialsproject:master Jan 17, 2025
42 checks passed
@kavanase kavanase deleted the minor_FermiDos_robustness_update branch January 17, 2025 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting user Needs more information from OP.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants