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 9 commits into
base: main
Choose a base branch
from
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 @@

: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 = (

Check warning on line 229 in src/aiida/orm/nodes/data/array/kpoints.py

View check run for this annotation

Codecov / codecov/patch

src/aiida/orm/nodes/data/array/kpoints.py#L229

Added line #L229 was not covered by tests
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

Check warning on line 234 in src/aiida/orm/nodes/data/array/kpoints.py

View check run for this annotation

Codecov / codecov/patch

src/aiida/orm/nodes/data/array/kpoints.py#L233-L234

Added lines #L233 - L234 were not covered by tests

if not isinstance(structuredata, AtomisticStructureData):
raise ValueError(error_message)

Check warning on line 237 in src/aiida/orm/nodes/data/array/kpoints.py

View check run for this annotation

Codecov / codecov/patch

src/aiida/orm/nodes/data/array/kpoints.py#L236-L237

Added lines #L236 - L237 were not covered by tests
else:
cell = structuredata.cell
self.set_cell(cell, structuredata.pbc)

Check warning on line 240 in src/aiida/orm/nodes/data/array/kpoints.py

View check run for this annotation

Codecov / codecov/patch

src/aiida/orm/nodes/data/array/kpoints.py#L239-L240

Added lines #L239 - L240 were not covered by tests
else:
raise ValueError(error_message)

Check warning on line 242 in src/aiida/orm/nodes/data/array/kpoints.py

View check run for this annotation

Codecov / codecov/patch

src/aiida/orm/nodes/data/array/kpoints.py#L242

Added line #L242 was not covered by tests
mikibonacci marked this conversation as resolved.
Show resolved Hide resolved
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
38 changes: 38 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 @@
return True


def has_atomistic():
mikibonacci marked this conversation as resolved.
Show resolved Hide resolved
""":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

Check warning on line 111 in src/aiida/orm/nodes/data/structure.py

View check run for this annotation

Codecov / codecov/patch

src/aiida/orm/nodes/data/structure.py#L111

Added line #L111 was not covered by tests
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,35 @@
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(

Check warning on line 1893 in src/aiida/orm/nodes/data/structure.py

View check run for this annotation

Codecov / codecov/patch

src/aiida/orm/nodes/data/structure.py#L1892-L1893

Added lines #L1892 - L1893 were not covered by tests
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 as AtomisticStructureData
from aiida_atomistic import StructureDataMutable as AtomisticStructureDataMutable

Check warning on line 1899 in src/aiida/orm/nodes/data/structure.py

View check run for this annotation

Codecov / codecov/patch

src/aiida/orm/nodes/data/structure.py#L1898-L1899

Added lines #L1898 - L1899 were not covered by tests
mikibonacci marked this conversation as resolved.
Show resolved Hide resolved

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

Check warning on line 1903 in src/aiida/orm/nodes/data/structure.py

View check run for this annotation

Codecov / codecov/patch

src/aiida/orm/nodes/data/structure.py#L1901-L1903

Added lines #L1901 - L1903 were not covered by tests

for site in self.sites:
atomistic.add_atom(

Check warning on line 1906 in src/aiida/orm/nodes/data/structure.py

View check run for this annotation

Codecov / codecov/patch

src/aiida/orm/nodes/data/structure.py#L1905-L1906

Added lines #L1905 - L1906 were not covered by tests
**{
'symbols': self.get_kind(site.kind_name).symbol,
'masses': self.get_kind(site.kind_name).mass,
'positions': site.position,
'kinds': site.kind_name,
}
)
mikibonacci marked this conversation as resolved.
Show resolved Hide resolved

return AtomisticStructureData.from_mutable(atomistic)

Check warning on line 1915 in src/aiida/orm/nodes/data/structure.py

View check run for this annotation

Codecov / codecov/patch

src/aiida/orm/nodes/data/structure.py#L1915

Added line #L1915 was not covered by tests


class Kind:
"""This class contains the information about the species (kinds) of the system.
Expand Down
79 changes: 79 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,79 @@ 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):
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 here you want to create a structure to be shared for many sub-tests. It would be better to use fixture.
The profile is more suitable for if you need to test the store and read data nodes from DB. If you need profile, there is aiida_profile fixture to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @unkcpz, would it be better to put also the last two tests in the above TestKpoints class but marked with the skip_atomistic fixture, and then in the init profile add a self.atomistic = self.structure.to_atomistic which run only if has_atomistic?

Copy link
Member

Choose a reason for hiding this comment

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

would it be better to put also the last two tests in the above TestKpoints class but marked with the skip_atomistic fixture

Yes, I think that'll be better!

"""Initialize the profile."""

from aiida_atomistic import StructureData as AtomisticStructureData
mikibonacci marked this conversation as resolved.
Show resolved Hide resolved
from aiida_atomistic import 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 = AtomisticStructureData.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
25 changes: 25 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,29 @@ def test_clone(self):
assert round(abs(c.sites[1].position[i] - 1.0), 7) == 0


@skip_atomistic
class TestAtomisticStructureData:
mikibonacci marked this conversation as resolved.
Show resolved Hide resolved
"""Tests the creation of StructureData objects (cell and pbc), and its conversion to the new atomistic StructureData."""

def test_to_atomistic(self):
"""Test the conversion 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