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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suspected Typo Fix in pymatgen.io.vasp.optics #2989

Merged
merged 3 commits into from
May 14, 2023

Conversation

kavanase
Copy link
Contributor

Hi!
Firstly, thanks to the developers for the recent addition of the pymatgen.io.vasp.optics module, it's been very very useful! 馃檶

This is just a small fix for a suspected typo in handling ismear in the delta_func() and step_func() functions. ISMEAR is zero for Gaussian smearing, and indeed in the docstrings for get_epsilon() it states: ismear: Smearing method (only has 0:gaussian, >0:Methfessel-Paxton). However, in the current version of the code:

def delta_func(x, ismear):
    """Replication of VASP's delta function"""
    if ismear < -1:
        raise ValueError("Delta function not implemented for ismear < -1")
    if ismear == -1:
        return step_func(x, -1) * (1 - step_func(x, -1))
    if ismear < 0:
        return np.exp(-(x * x)) / np.sqrt(np.pi)
    return delta_methfessel_paxton(x, ismear)

ismear = 0 ends up returning the Methfessel-Paxton smearing (delta_methfessel_paxton), however the function under if ismear < 0 is the Gaussian smearing. ismear should be an integer anyway, so having ismear < -1 raise an error and ismear == -1 return something else, it means that the ismear < 0 line is always skipped. This is also the case for the step_func() function.

I believe this is likely a small typo, and should instead by if ismear == 0:. I have changed it to this in this PR, and tests are passing.

@janosh
Copy link
Member

janosh commented May 14, 2023

Thanks for the fix! 馃憤 This sounds sensible. We should definitely have tests for these functions.

@janosh janosh added fix Bug fix PRs io Input/output functionality labels May 14, 2023
@janosh janosh enabled auto-merge (squash) May 14, 2023 14:58
@janosh janosh merged commit 82ea137 into materialsproject:master May 14, 2023
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix PRs io Input/output functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants