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

pymatgen structure creation patch #305

Merged
merged 4 commits into from
Oct 10, 2024

Conversation

laserkelvin
Copy link
Collaborator

@laserkelvin laserkelvin commented Oct 10, 2024

This PR resolves #304 by refactoring PeriodicPropertiesTransform in two ways:

  1. We will reuse a serialized structure if it is interpreted as a pymatgen.core.Structure for the periodic boundary neighbors calculation, and return directly.
  2. If the above condition fails, we'll continue down the old path, but Lattice.from_parameters is now passed vesta=True in all cases if creating from lattice parameters. This set up to happen in both the transform, as well as the subroutine matsciml.datasets.utils.make_pymatgen_periodic_structure.

The impact of this change is likely only with datasets that do not have a cell attribute in their data. e.g. LiPSDataset and Open Catalyst expose cell which is used for the Lattice creation, and if the fully qualified lattice matrix is given then the resulting lattice is consistent. Only when no cell is available is when Lattice.from_parameters is called.

A related issue is #267, if and when we get to it. There would not be inconsistencies if the pymatgen.core.Structure is created from first principles, as opposed to loading a serialized one.

@laserkelvin laserkelvin added bug Something isn't working data Issues related to data loading, pipelining, etc. labels Oct 10, 2024
Copy link
Collaborator

@melo-gonzo melo-gonzo 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 the updates. Glad this is all sorted.

@laserkelvin laserkelvin merged commit b66d4c3 into IntelLabs:main Oct 10, 2024
2 of 3 checks passed
@laserkelvin laserkelvin deleted the pmg-structure-creation-patch branch October 10, 2024 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working data Issues related to data loading, pipelining, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Lattice objects created are inconsistent with existing ones
2 participants