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 the pbc constraints of get_pymatgen_structure #6281

Merged
merged 12 commits into from
Feb 12, 2024

Conversation

bastonero
Copy link
Contributor

Fixes #6280

The pbc for getting a pymatgen.core.structure.Structure class via the StructureData.get_pymatgen_structure were constrained to be 3D, i.e. (True,True,True). The core module of pymatgen actually allows for any pbc condition, and this is now fixed by using the pymatgen.core.structure.Lattice class to specify, along the cell information, also the pbc.

This is useful in many other codes that exploit pymatgen core utilities, and overcomes this huge limitation, thus avoiding patches and possible shortcomings.

The pbc for getting a `pymatgen.core.structure.Structure`
class via the `StructureData.get_pymatgen_structure` were
constrained to be 3D, i.e. (True,True,True). The core module
of pymatgen actually allows for any pbc condition, and this
is now fixed by using the `pymatgen.core.structure.Lattice`
class to specify, along the cell information, also the pbc.

This is useful in many other codes that exploit pymatgen core
utilities, and overcomes this huge limitation, thus avoiding
patches and possible shortcomings.
Copy link
Collaborator

@danielhollas danielhollas 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 doing this! One question about requiring default cell...

tests/test_dataclasses.py Show resolved Hide resolved
Copy link
Collaborator

@danielhollas danielhollas left a comment

Choose a reason for hiding this comment

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

LGTM! Just a couple of wording suggestions.

src/aiida/orm/nodes/data/structure.py Outdated Show resolved Hide resolved
src/aiida/orm/nodes/data/structure.py Outdated Show resolved Hide resolved
src/aiida/orm/nodes/data/structure.py Outdated Show resolved Hide resolved
@sphuber
Copy link
Contributor

sphuber commented Feb 11, 2024

Thanks @bastonero . I think this PR has been reviewed and implicitly approved? Just the docs are failing with the following warnings:

/home/docs/checkouts/readthedocs.org/user_builds/aiida-core/envs/6281/lib/python3.10/site-packages/aiida/orm/nodes/data/structure.py:docstring of aiida.orm.nodes.data.structure.StructureData.get_pymatgen:7: WARNING: Field list ends without a blank line; unexpected unindent.
/home/docs/checkouts/readthedocs.org/user_builds/aiida-core/envs/6281/lib/python3.10/site-packages/aiida/orm/nodes/data/structure.py:docstring of aiida.orm.nodes.data.structure.StructureData.get_pymatgen_structure:4: WARNING: Field list ends without a blank line; unexpected unindent.
/home/docs/checkouts/readthedocs.org/user_builds/aiida-core/envs/6281/lib/python3.10/site-packages/aiida/orm/nodes/data/structure.py:docstring of aiida.orm.nodes.data.structure.StructureData.get_pymatgen:7: WARNING: Field list ends without a blank line; unexpected unindent.
/home/docs/checkouts/readthedocs.org/user_builds/aiida-core/envs/6281/lib/python3.10/site-packages/aiida/orm/nodes/data/structure.py:docstring of aiida.orm.nodes.data.structure.StructureData.get_pymatgen_structure:4: WARNING: Field list ends without a blank line; unexpected unindent.

If you could fix those, this can be merged. Thanks

@sphuber sphuber self-requested a review February 12, 2024 08:22
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks @bastonero

@sphuber sphuber merged commit adcce4b into aiidateam:main Feb 12, 2024
19 checks passed
@bastonero bastonero deleted the fix/pymatgen-pbc branch March 19, 2024 09:05
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.

The get_pymatgen_structure doesn't allow pbc != 3D
4 participants