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

Providing support for atomistic StructureData #6632

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 17 additions & 7 deletions src/aiida/orm/nodes/data/array/kpoints.py
Original file line number Diff line number Diff line change
Expand Up @@ -223,16 +223,26 @@ def set_cell_from_structure(self, structuredata):
:param structuredata: an instance of StructureData
"""
from aiida.orm import StructureData
from aiida.orm.nodes.data.structure import StructureData, has_atomistic
mikibonacci marked this conversation as resolved.
Show resolved Hide resolved

if not isinstance(structuredata, StructureData):
raise ValueError(
'An instance of StructureData should be passed to ' 'the KpointsData, found instead {}'.format(
structuredata.__class__
)
error_message = (
Copy link
Member

Choose a reason for hiding this comment

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

I think this can directly move to raise exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'An instance of StructureData or aiida-atomistic StructureData should be passed to '
'the KpointsData, found instead {}'.format(structuredata.__class__)
)
cell = structuredata.cell
self.set_cell(cell, structuredata.pbc)
if has_atomistic():
from aiida_atomistic import StructureData as AtomisticStructureData

if not isinstance(structuredata, AtomisticStructureData):
raise ValueError(error_message)
else:
cell = structuredata.cell
self.set_cell(cell, structuredata.pbc)
else:
raise ValueError(error_message)
Comment on lines +241 to +242
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it is possible to move the exception to same condition and move the set_cell to same condition.

Copy link
Contributor Author

@mikibonacci mikibonacci Nov 23, 2024

Choose a reason for hiding this comment

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

Hi @unkcpz, thank you for the comment. I have actually another idea, which is replacing the whole block from line 228 to 245 with:

if has_atomistic():
    from aiida_atomistic import StructureData as AtomisticStructureData

if not isinstance(structuredata, StructureData) and not isinstance(structuredata, AtomisticStructureData if has_atomistic() else StructureData):
    raise ValueError('An instance of StructureData or aiida-atomistic StructureData should be passed to '
                'the KpointsData, found instead {}'.format(structuredata.__class__))
else:
    cell = structuredata.cell
    self.set_cell(cell, structuredata.pbc)

This seems redundant, but actually makes more compact the check. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I guess if the structuredata is neither StructureData nor the AtomisticStructureData, the else block of calling structuredata.cell and self.set_cell with structuredata.pbc will fail?
If so, why not use try..except block and put the current else block in the try and the catch the exceptions for correspond operations? Let me know if I am missing something and we can try it together in person.

Copy link
Contributor Author

@mikibonacci mikibonacci Nov 23, 2024

Choose a reason for hiding this comment

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

In the way you suggest, if structuredata was an arbitrary object which defines cell and pbc as attributes, the exceptions will not be raised but it should. And the user will not see the error (i.e. not providing an orm or atomistic StructureData).

Anyway, let's discuss this in person on monday

else:
cell = structuredata.cell
self.set_cell(cell, structuredata.pbc)

def set_cell(self, cell, pbc=None):
"""Set a cell to be used for symmetry analysis.
Expand Down
35 changes: 35 additions & 0 deletions src/aiida/orm/nodes/data/structure.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,15 @@ def has_pymatgen():
return True


def has_atomistic() -> bool:
""":return: True if the StructureData and StructureDataMutable from aiida-atomistic module can be imported, False otherwise."""
try:
import aiida_atomistic # noqa: F401
except ImportError:
return False
return True
mikibonacci marked this conversation as resolved.
Show resolved Hide resolved


def get_pymatgen_version():
""":return: string with pymatgen version, None if can not import."""
if not has_pymatgen():
Expand Down Expand Up @@ -1876,6 +1885,32 @@ def _get_object_pymatgen_molecule(self, **kwargs):
positions = [list(site.position) for site in self.sites]
return Molecule(species, positions)

def to_atomistic(self):
"""
Returns the atomistic StructureData version of the orm.StructureData one.
"""
if not has_atomistic():
raise ImportError(
mikibonacci marked this conversation as resolved.
Show resolved Hide resolved
'aiida-atomistic plugin is not installed, \
please install it to have full support for atomistic structures'
)
else:
from aiida_atomistic import StructureData, StructureDataMutable

atomistic = StructureDataMutable()
atomistic.set_pbc(self.pbc)
atomistic.set_cell(self.cell)

for site in self.sites:
atomistic.add_atom(
symbols=self.get_kind(site.kind_name).symbol,
masses=self.get_kind(site.kind_name).mass,
positions=site.position,
kinds=site.kind_name,
)

return StructureData.from_mutable(atomistic)


class Kind:
"""This class contains the information about the species (kinds) of the system.
Expand Down
78 changes: 78 additions & 0 deletions tests/orm/nodes/data/test_kpoints.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@
import numpy as np
import pytest
from aiida.orm import KpointsData, StructureData, load_node
from aiida.orm.nodes.data.structure import has_atomistic

skip_atomistic = pytest.mark.skipif(not has_atomistic(), reason='Unable to import aiida-atomistic')


class TestKpoints:
Expand Down Expand Up @@ -81,3 +84,78 @@ def test_get_kpoints(self):
kpt2 = load_node(kpt.pk)
assert np.abs(kpt2.get_kpoints() - np.array(kpoints)).sum() == 0.0
assert np.abs(kpt2.get_kpoints(cartesian=True) - np.array(cartesian_kpoints)).sum() == 0.0


@skip_atomistic
class TestKpointsAtomisticStructureData:
"""Test for the `Kpointsdata` class using the new atomistic StructureData."""

@pytest.fixture(autouse=True)
def init_profile(self):
unkcpz marked this conversation as resolved.
Show resolved Hide resolved
"""Initialize the profile."""

from aiida_atomistic import StructureData, StructureDataMutable

alat = 5.430 # angstrom
cell = [
[
0.5 * alat,
0.5 * alat,
0.0,
],
[
0.0,
0.5 * alat,
0.5 * alat,
],
[0.5 * alat, 0.0, 0.5 * alat],
]
self.alat = alat
mutable = StructureDataMutable()
mutable.set_cell(cell)
mutable.add_atom(positions=(0.000 * alat, 0.000 * alat, 0.000 * alat), symbols='Si')
mutable.add_atom(positions=(0.250 * alat, 0.250 * alat, 0.250 * alat), symbols='Si')
self.structure = StructureData.from_mutable(mutable)
# Define the expected reciprocal cell
val = 2.0 * np.pi / alat
self.expected_reciprocal_cell = np.array([[val, val, -val], [-val, val, val], [val, -val, val]])

def test_reciprocal_cell(self):
"""Test the `reciprocal_cell` method.

This is a regression test for #2749.
"""
kpt = KpointsData()
kpt.set_cell_from_structure(self.structure)

assert np.abs(kpt.reciprocal_cell - self.expected_reciprocal_cell).sum() == 0.0

# Check also after storing
kpt.store()
kpt2 = load_node(kpt.pk)
assert np.abs(kpt2.reciprocal_cell - self.expected_reciprocal_cell).sum() == 0.0

def test_get_kpoints(self):
"""Test the `get_kpoints` method."""
kpt = KpointsData()
kpt.set_cell_from_structure(self.structure)

kpoints = [
[0.0, 0.0, 0.0],
[0.5, 0.5, 0.5],
]

cartesian_kpoints = [
[0.0, 0.0, 0.0],
[np.pi / self.alat, np.pi / self.alat, np.pi / self.alat],
]

kpt.set_kpoints(kpoints)
assert np.abs(kpt.get_kpoints() - np.array(kpoints)).sum() == 0.0
assert np.abs(kpt.get_kpoints(cartesian=True) - np.array(cartesian_kpoints)).sum() == 0.0

# Check also after storing
kpt.store()
kpt2 = load_node(kpt.pk)
assert np.abs(kpt2.get_kpoints() - np.array(kpoints)).sum() == 0.0
assert np.abs(kpt2.get_kpoints(cartesian=True) - np.array(cartesian_kpoints)).sum() == 0.0
22 changes: 22 additions & 0 deletions tests/test_dataclasses.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
get_formula,
get_pymatgen_version,
has_ase,
has_atomistic,
has_pymatgen,
has_spglib,
)
Expand Down Expand Up @@ -67,6 +68,7 @@ def simplify(string):
skip_spglib = pytest.mark.skipif(not has_spglib(), reason='Unable to import spglib')
skip_pycifrw = pytest.mark.skipif(not has_pycifrw(), reason='Unable to import PyCifRW')
skip_pymatgen = pytest.mark.skipif(not has_pymatgen(), reason='Unable to import pymatgen')
skip_atomistic = pytest.mark.skipif(not has_atomistic(), reason='Unable to import aiida-atomistic')


@skip_pymatgen
Expand Down Expand Up @@ -1851,6 +1853,26 @@ def test_clone(self):
assert round(abs(c.sites[1].position[i] - 1.0), 7) == 0


@skip_atomistic
def test_to_atomistic(self):
"""Test the conversion from orm.StructureData to the atomistic structure."""

# Create a structure with a single atom
from aiida_atomistic import StructureData as AtomisticStructureData

legacy = StructureData(cell=((1.0, 0.0, 0.0), (0.0, 2.0, 0.0), (0.0, 0.0, 3.0)))
legacy.append_atom(position=(0.0, 0.0, 0.0), symbols=['Ba'], name='Ba1')

# Convert to atomistic structure
structure = legacy.to_atomistic()

# Check that the structure is as expected
assert isinstance(structure, AtomisticStructureData)
assert structure.properties.sites[0].kinds == legacy.sites[0].kind_name
assert structure.properties.sites[0].positions == list(legacy.sites[0].position)
assert structure.properties.cell == legacy.cell


class TestStructureDataFromAse:
"""Tests the creation of Sites from/to a ASE object."""

Expand Down
Loading