From 950279fb5c274e3411ad743a47cc0d560ce8c826 Mon Sep 17 00:00:00 2001 From: Marnik Bercx Date: Mon, 12 Feb 2024 17:47:53 +0100 Subject: [PATCH 1/2] `HubbardStructureData`: Add validation on site indices The `append_hubbard_parameter()` method of the `HubbardStructureData` class currently does not verify that the `atom_index` and `neighbour_index` values correspond to valid site indices in the structure. This means that the method will: - fail in case `translation` is set to `None` (as it attempts to access these site indices to obtain the distance and image of the nearest neighbour using periodic boundary conditions) - succeed in case the translation is specified, but fail once the code tries to write Hubbard card during the `prepare_for_submission` method of the `PwCalculation`. To have more consistent behaviour and avoid exceptions during the `upload` step of the submission process, we check here that the `atom_index` and `neighbour_index` are valid and raise a `ValueError` if they are not. --- .../data/hubbard_structure.py | 6 +++ tests/conftest.py | 4 ++ tests/data/test_hubbard_structure.py | 43 +++++++++++++++---- ...ubbard_parameters_parameters0_silicon_.yml | 9 ++++ ...ubbard_parameters_parameters1_silicon_.yml | 9 ++++ ...ubbard_parameters_parameters2_silicon_.yml | 9 ++++ ...ubbard_parameters_parameters3_silicon_.yml | 18 ++++++++ 7 files changed, 90 insertions(+), 8 deletions(-) create mode 100644 tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters0_silicon_.yml create mode 100644 tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters1_silicon_.yml create mode 100644 tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters2_silicon_.yml create mode 100644 tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters3_silicon_.yml diff --git a/src/aiida_quantumespresso/data/hubbard_structure.py b/src/aiida_quantumespresso/data/hubbard_structure.py index 78d0862b4..f41457a0f 100644 --- a/src/aiida_quantumespresso/data/hubbard_structure.py +++ b/src/aiida_quantumespresso/data/hubbard_structure.py @@ -58,6 +58,7 @@ def hubbard(self) -> Hubbard: :returns: a :class:`~aiida_quantumespresso.common.hubbard.Hubbard` instance. """ + # pylint: disable=not-context-manager with self.base.repository.open(self._hubbard_filename, mode='rb') as handle: return Hubbard.model_validate_json(json.load(handle)) @@ -112,6 +113,11 @@ def append_hubbard_parameter( pymat = self.get_pymatgen_structure() sites = pymat.sites + if any((atom_index > len(sites) - 1, neighbour_index > len(sites) - 1)): + raise ValueError( + 'atom_index and neighbour_index must be within the range of the number of sites in the structure' + ) + if translation is None: _, translation = sites[atom_index].distance_and_image(sites[neighbour_index]) translation = np.array(translation, dtype=np.int64).tolist() diff --git a/tests/conftest.py b/tests/conftest.py index c208e09eb..8f9fffa32 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -375,6 +375,10 @@ def _generate_structure(structure_id='silicon'): structure = StructureData(cell=cell) structure.append_atom(position=(0., 0., 0.), symbols='Si', name=name1) structure.append_atom(position=(param / 4., param / 4., param / 4.), symbols='Si', name=name2) + elif structure_id == 'cobalt-prim': + cell = [[0.0, 2.715, 2.715], [2.715, 0.0, 2.715], [2.715, 2.715, 0.0]] + structure = StructureData(cell=cell) + structure.append_atom(position=(0.0, 0.0, 0.0), symbols='Co', name='Co') elif structure_id == 'water': structure = StructureData(cell=[[5.29177209, 0., 0.], [0., 5.29177209, 0.], [0., 0., 5.29177209]]) structure.append_atom(position=[12.73464656, 16.7741411, 24.35076238], symbols='H', name='H') diff --git a/tests/data/test_hubbard_structure.py b/tests/data/test_hubbard_structure.py index 7cd9ae032..812bda404 100644 --- a/tests/data/test_hubbard_structure.py +++ b/tests/data/test_hubbard_structure.py @@ -62,15 +62,42 @@ def test_from_structure(generate_structure, generate_hubbard): @pytest.mark.usefixtures('aiida_profile') -def test_append_hubbard_parameters(generate_hubbard_structure): +@pytest.mark.parametrize('structure_name', ('silicon',)) +@pytest.mark.parametrize( + 'parameters', ( + ((0, '1s', 0, '1s', 5.0, (0, 0, 0), 'Ueff'),), + ((0, '1s', 1, '1s', 5.0, None, 'V'),), + ( + (0, '1s', 0, '1s', 5.0, (0, 0, 0), 'Ueff'), + (0, '1s', 0, '1s', 5.0, (0, 0, 0), 'Ueff'), + ), + ( + (0, '1s', 0, '1s', 5.0, (0, 0, 0), 'Ueff'), + (0, '1s', 1, '1s', 5.0, (0, 1, 0), 'V'), + ), + ) +) +def test_append_hubbard_parameters(data_regression, generate_structure, structure_name, parameters): """Test the `append_hubbard_parameters` method.""" - from aiida_quantumespresso.common.hubbard import HubbardParameters - hubbard_structure = generate_hubbard_structure() - args = (0, '1s', 1, '1s', 5.0, (0, 0, 0), 'U') - hubbard_structure.append_hubbard_parameter(*args) - params = HubbardParameters.from_tuple(args) - assert len(hubbard_structure.hubbard.parameters) == 2 - assert params == hubbard_structure.hubbard.parameters[1] + hubbard_structure = HubbardStructureData.from_structure(generate_structure(structure_name)) + + for parameter in parameters: + hubbard_structure.append_hubbard_parameter(*parameter) + + data_regression.check(hubbard_structure.hubbard.to_list()) + assert len(hubbard_structure.hubbard.parameters) == len(set(parameters)) + + +@pytest.mark.parametrize('parameter', ( + (0, '1s', 1, '1s', 5.0, None, 'V'), + (0, '1s', 1, '1s', 5.0, (0, 0, 0), 'V'), +)) +def test_append_hubbard_parameters_invalid_index(generate_structure, parameter): + """Test the `append_hubbard_parameters` method with invalid index.""" + hubbard_structure = HubbardStructureData.from_structure(generate_structure('cobalt-prim')) + + with pytest.raises(ValueError, match='atom_index and neighbour_index must be within the range'): + hubbard_structure.append_hubbard_parameter(*parameter) @pytest.mark.usefixtures('aiida_profile') diff --git a/tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters0_silicon_.yml b/tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters0_silicon_.yml new file mode 100644 index 000000000..af0e1049e --- /dev/null +++ b/tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters0_silicon_.yml @@ -0,0 +1,9 @@ +- - 0 + - 1s + - 0 + - 1s + - 5.0 + - - 0 + - 0 + - 0 + - Ueff diff --git a/tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters1_silicon_.yml b/tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters1_silicon_.yml new file mode 100644 index 000000000..23a8bcbed --- /dev/null +++ b/tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters1_silicon_.yml @@ -0,0 +1,9 @@ +- - 0 + - 1s + - 1 + - 1s + - 5.0 + - - -1 + - 0 + - 0 + - V diff --git a/tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters2_silicon_.yml b/tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters2_silicon_.yml new file mode 100644 index 000000000..af0e1049e --- /dev/null +++ b/tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters2_silicon_.yml @@ -0,0 +1,9 @@ +- - 0 + - 1s + - 0 + - 1s + - 5.0 + - - 0 + - 0 + - 0 + - Ueff diff --git a/tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters3_silicon_.yml b/tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters3_silicon_.yml new file mode 100644 index 000000000..59b94e600 --- /dev/null +++ b/tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters3_silicon_.yml @@ -0,0 +1,18 @@ +- - 0 + - 1s + - 0 + - 1s + - 5.0 + - - 0 + - 0 + - 0 + - Ueff +- - 0 + - 1s + - 1 + - 1s + - 5.0 + - - 0 + - 1 + - 0 + - V From e1903763728c03b49ea5067abd0b029a039a7b94 Mon Sep 17 00:00:00 2001 From: Marnik Bercx Date: Mon, 12 Feb 2024 18:37:36 +0100 Subject: [PATCH 2/2] `HubbardStructureData`: add compatibility for <3D structures The `HubbardStructureData.append_hubbard_parameter()` method currently fails for structures that don't have 3-dimensional periodic boundary conditions. This is caused by a restriction in the StructureData.get_pymatgen_structure()` method, which raises a `ValueError` if the periodic boundary conditions are not (True, True, True). This has been fixed in a recent commit on `aiida-core`: https://github.com/aiidateam/aiida-core/commit/adcce4bcd0b59c8371be73058a060bedcaba40f6 But this will take some time to be released and would restrict our compatibility to `aiida-core>=2.6.0`. Here we directly construct a list of `PeriodicSite` objects from the `sites` list of the object obtained from `StructureData.get_pymatgen()`, which can be both either a pymatgen `Structure` or `Molecule`, depending on the dimensionality of the structure. Co-authored-by: AndresOrtegaGuerrero <34098967+AndresOrtegaGuerrero@users.noreply.github.com> --- .../data/hubbard_structure.py | 11 +++++++++-- tests/data/test_hubbard_structure.py | 7 ++++--- ...d_parameters_parameters0_2D_xy_arsenic_.yml | 9 +++++++++ ...d_parameters_parameters1_2D_xy_arsenic_.yml | 9 +++++++++ ...d_parameters_parameters2_2D_xy_arsenic_.yml | 9 +++++++++ ...d_parameters_parameters3_2D_xy_arsenic_.yml | 18 ++++++++++++++++++ 6 files changed, 58 insertions(+), 5 deletions(-) create mode 100644 tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters0_2D_xy_arsenic_.yml create mode 100644 tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters1_2D_xy_arsenic_.yml create mode 100644 tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters2_2D_xy_arsenic_.yml create mode 100644 tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters3_2D_xy_arsenic_.yml diff --git a/src/aiida_quantumespresso/data/hubbard_structure.py b/src/aiida_quantumespresso/data/hubbard_structure.py index f41457a0f..a5c8d2b63 100644 --- a/src/aiida_quantumespresso/data/hubbard_structure.py +++ b/src/aiida_quantumespresso/data/hubbard_structure.py @@ -5,6 +5,7 @@ from aiida.orm import StructureData import numpy as np +from pymatgen.core import Lattice, PeriodicSite from aiida_quantumespresso.common.hubbard import Hubbard, HubbardParameters @@ -110,8 +111,14 @@ def append_hubbard_parameter( :param hubbard_type: hubbard type (U, V, J, ...), defaults to 'Ueff' (see :class:`~aiida_quantumespresso.common.hubbard.Hubbard` for full allowed values) """ - pymat = self.get_pymatgen_structure() - sites = pymat.sites + sites = [ + PeriodicSite( + species=site.species, + coords=site.coords, + lattice=Lattice(self.cell, pbc=self.pbc), + coords_are_cartesian=True + ) for site in self.get_pymatgen().sites + ] if any((atom_index > len(sites) - 1, neighbour_index > len(sites) - 1)): raise ValueError( diff --git a/tests/data/test_hubbard_structure.py b/tests/data/test_hubbard_structure.py index 812bda404..b07860cfc 100644 --- a/tests/data/test_hubbard_structure.py +++ b/tests/data/test_hubbard_structure.py @@ -62,7 +62,7 @@ def test_from_structure(generate_structure, generate_hubbard): @pytest.mark.usefixtures('aiida_profile') -@pytest.mark.parametrize('structure_name', ('silicon',)) +@pytest.mark.parametrize('structure_name', ('silicon', '2D-xy-arsenic')) @pytest.mark.parametrize( 'parameters', ( ((0, '1s', 0, '1s', 5.0, (0, 0, 0), 'Ueff'),), @@ -88,13 +88,14 @@ def test_append_hubbard_parameters(data_regression, generate_structure, structur assert len(hubbard_structure.hubbard.parameters) == len(set(parameters)) +@pytest.mark.parametrize('structure_name', ('cobalt-prim', '1D-x-carbon')) @pytest.mark.parametrize('parameter', ( (0, '1s', 1, '1s', 5.0, None, 'V'), (0, '1s', 1, '1s', 5.0, (0, 0, 0), 'V'), )) -def test_append_hubbard_parameters_invalid_index(generate_structure, parameter): +def test_append_hubbard_parameters_invalid_index(generate_structure, structure_name, parameter): """Test the `append_hubbard_parameters` method with invalid index.""" - hubbard_structure = HubbardStructureData.from_structure(generate_structure('cobalt-prim')) + hubbard_structure = HubbardStructureData.from_structure(generate_structure(structure_name)) with pytest.raises(ValueError, match='atom_index and neighbour_index must be within the range'): hubbard_structure.append_hubbard_parameter(*parameter) diff --git a/tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters0_2D_xy_arsenic_.yml b/tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters0_2D_xy_arsenic_.yml new file mode 100644 index 000000000..af0e1049e --- /dev/null +++ b/tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters0_2D_xy_arsenic_.yml @@ -0,0 +1,9 @@ +- - 0 + - 1s + - 0 + - 1s + - 5.0 + - - 0 + - 0 + - 0 + - Ueff diff --git a/tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters1_2D_xy_arsenic_.yml b/tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters1_2D_xy_arsenic_.yml new file mode 100644 index 000000000..577587c2c --- /dev/null +++ b/tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters1_2D_xy_arsenic_.yml @@ -0,0 +1,9 @@ +- - 0 + - 1s + - 1 + - 1s + - 5.0 + - - 0 + - 0 + - 0 + - V diff --git a/tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters2_2D_xy_arsenic_.yml b/tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters2_2D_xy_arsenic_.yml new file mode 100644 index 000000000..af0e1049e --- /dev/null +++ b/tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters2_2D_xy_arsenic_.yml @@ -0,0 +1,9 @@ +- - 0 + - 1s + - 0 + - 1s + - 5.0 + - - 0 + - 0 + - 0 + - Ueff diff --git a/tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters3_2D_xy_arsenic_.yml b/tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters3_2D_xy_arsenic_.yml new file mode 100644 index 000000000..59b94e600 --- /dev/null +++ b/tests/data/test_hubbard_structure/test_append_hubbard_parameters_parameters3_2D_xy_arsenic_.yml @@ -0,0 +1,18 @@ +- - 0 + - 1s + - 0 + - 1s + - 5.0 + - - 0 + - 0 + - 0 + - Ueff +- - 0 + - 1s + - 1 + - 1s + - 5.0 + - - 0 + - 1 + - 0 + - V