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

Fix grid overlap #4

Closed
wants to merge 1 commit into from
Closed

Fix grid overlap #4

wants to merge 1 commit into from

Conversation

fxcoudert
Copy link
Contributor

This fixes a bug in the current code, where grid points at the edges ( x = 1 or y = 1 or z = 1 , in fractional coordinates) are included, which leads to duplication (because they are the same as x = 0 , etc).

Given the use of periodic boundary conditions for the voxels, duplication is wrong. It leads to give double weight to points on the faces of the cell, and leads to wrong PBC application once the voxels are processed.

np.linspace() accepts a endpoint=False optional arg, which keeps the total number of points but does not include the endpoint. See in this simple example:

>>> import numpy as np
>>> np.linspace(0, 1, 5)
array([0.  , 0.25, 0.5 , 0.75, 1.  ])
>>> np.linspace(0, 1, 5, endpoint=False)
array([0. , 0.2, 0.4, 0.6, 0.8])

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@adosar
Copy link
Owner

adosar commented May 6, 2024

This should not affect the case where cubic_box is set to True (cartesian coords are used in this case), right? Meaning that only the fractional coordinates case should be modified, i.e.:

if cubic_box:
    d = length / 2
    probe_coords = np.linspace(0-d, 0+d, self.grid_size)  # Cartesian, leave it as is.
    self._simulation_box = self.structure
else:
    probe_coords = np.linspace(0, 1, self.grid_size, endpoint=False)  # Fractional.

@fxcoudert
Copy link
Contributor Author

Hum, I wonder. If you set cubic_box and the box is not cubic, you're definitely biasing the sampling anyway. So maybe it does not matter much if you bias more?

On the other hand, if you set cubic_box but the box is actually cubic then you're actually still giving double weight to those points. That, at least, should be avoided.

I don't have a strong opinion. But I don't think cubic_box is physically meaningful anyway… have you tested whether it improves accuracy of the trained models?

@adosar
Copy link
Owner

adosar commented May 6, 2024

On the other hand, if you set cubic_box but the box is actually cubic then you're actually still giving double weight to those points. That, at least, should be avoided.

Yep, that is correct. To ensure correct PBC, the argument endpoint will be added in both cases.

I don't have a strong opinion. But I don't think cubic_box is physically meaningful anyway… have you tested whether it improves accuracy of the trained models?

I have not tested the performance of models with cubic_box set to True, but I believe it might be beneficial if the dataset under consideration contains many materials with large deviations from orthogonality.

@adosar adosar closed this in 5282be2 Mar 23, 2025
@fxcoudert fxcoudert deleted the grid branch March 23, 2025 18:06
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.

None yet

2 participants