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

Update inequality in get_linear_interpolated_value #4299

Merged

Conversation

kavanase
Copy link
Contributor

This is a small but important fix for the get_linear_interpolated_value function in pymatgen.util.coord, used in various places within the codebase.

This function works by finding the first index in x_values that is greater than / equal to x, and then uses this index and the previous (idx-1) with x_values and y_values to interpolate:
https://github.com/materialsproject/pymatgen/blob/v2025.2.18/src/pymatgen/util/coord.py#L136-L147

The issue is that if the first index in x_values is equal to x, this causes the ValueError to be thrown (as indices[0]==0); where it thinks that the input x to interpolate is outside the interpolable range. Changing the inequality to > rather than >= resolves this issue.

MWE:

from pymatgen.util.coord import get_linear_interpolated_value

get_linear_interpolated_value([0.0001, 0.0002], [1,2], 0.0001)

Current pymatgen master:
image
Where we can see "x=0.0001 is out of range of provided x_values (0.0001, 0.0002)" does not make sense.

This branch:
image

And confirming the error is still correctly thrown with an x value that is actually outside the range:
image

@kavanase
Copy link
Contributor Author

Also there was a pre-commit codespell failure due to a variable name in local_env which was fixed in 8fdea16

@kavanase
Copy link
Contributor Author

This also adds some very small updates to make the behaviour of DOS.get_interpolated_gap and Dos.get_interpolated_gap fully consistent

@mkhorton mkhorton enabled auto-merge (squash) February 25, 2025 07:25
Copy link
Member

@mkhorton mkhorton left a comment

Choose a reason for hiding this comment

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

Thanks for catching and fixing this @kavanase! Appreciate the code readability improvements too.

@mkhorton mkhorton disabled auto-merge February 25, 2025 07:28
@mkhorton mkhorton merged commit 195f19e into materialsproject:master Feb 25, 2025
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants