Apply review 2

Author: mbercx

src/aiida/orm/nodes/data/array/trajectory.py

@@ -12,8 +12,10 @@
 
 import collections.abc
 from typing import List
+from warnings import warn
 
 from aiida.common.pydantic import MetadataField
+from aiida.common.warnings import AiidaDeprecationWarning
 
 from .array import ArrayData
 
@@ -86,6 +88,8 @@ def _internal_validate(self, stepids, cells, symbols, positions, times, velociti
                 )
         if not (isinstance(pbc, (list, tuple)) and len(pbc) == 3 and all(isinstance(val, bool) for val in pbc)):
             raise ValueError('`pbc` must be a list/tuple of length three with boolean values.')
+        if cells is None and list(pbc) != [False, False, False]:
+            raise ValueError('Periodic boundary conditions are only possible when a cell is defined.')
 
     def set_trajectory(
         self, symbols, positions, stepids=None, cells=None, times=None, velocities=None, pbc: None | list | tuple = None
@@ -145,8 +149,15 @@ def set_trajectory(
         """
         import numpy
 
-        pbc_default = [True, True, True] if cells is not None else [False, False, False]
-        pbc = pbc or pbc_default
+        if cells is None:
+            pbc = pbc or [False, False, False]
+        elif pbc is None:
+            warn(
+                "When 'cells' is not None, the periodic boundary conditions should be explicitly specified via the "
+                "'pbc' keyword argument. Defaulting to '[True, True, True]', but this will raise in v3.0.0.",
+                AiidaDeprecationWarning,
+            )
+            pbc = [True, True, True]
 
         self._internal_validate(stepids, cells, symbols, positions, times, velocities, pbc)
         # set symbols/pbc as attributes for easier querying

tests/orm/nodes/data/test_trajectory.py

@@ -3,6 +3,7 @@
 import numpy as np
 import pytest
 
+from aiida.common.warnings import AiidaDeprecationWarning
 from aiida.orm import StructureData, TrajectoryData, load_node
 
 
@@ -147,8 +148,8 @@ def test_trajectory_get_step_structure(self, trajectory_data):
         with pytest.raises(IndexError):
             trajectory.get_step_structure(500)
 
-    def test_trajectory_pbc(self, trajectory_data):
-        """Test the `pbc` for the `TrajectoryData`."""
+    def test_trajectory_pbc_structures(self, trajectory_data):
+        """Test the `pbc` for the `TrajectoryData` using structure inputs."""
         # Test non-pbc structure with no cell
         structure = StructureData(cell=None, pbc=[False, False, False])
         structure.append_atom(position=[0.0, 0.0, 0.0], symbols='H')
@@ -174,3 +175,62 @@ def test_trajectory_pbc(self, trajectory_data):
 
         with pytest.raises(ValueError, match='All structures should have the same `pbc`'):
             TrajectoryData(structurelist=(structure_periodic, structure_non_periodic))
+
+    def test_trajectory_pbc_set_trajectory(self):
+        """Test the `pbc` for the `TrajectoryData` using `set_trajectory`."""
+        data = {
+            'symbols': ['H'],
+            'positions': np.array(
+                [
+                    [
+                        [0.0, 0.0, 0.0],
+                    ]
+                ]
+            ),
+        }
+        trajectory = TrajectoryData()
+
+        data.update(
+            {
+                'cells': None,
+                'pbc': None,
+            }
+        )
+        trajectory.set_trajectory(**data)
+        assert trajectory.get_step_structure(0).pbc == (False, False, False)
+
+        data.update(
+            {
+                'cells': None,
+                'pbc': [False, False, False],
+            }
+        )
+        trajectory.set_trajectory(**data)
+        assert trajectory.get_step_structure(0).pbc == (False, False, False)
+
+        data.update(
+            {
+                'cells': None,
+                'pbc': [True, False, False],
+            }
+        )
+        with pytest.raises(ValueError, match='Periodic boundary conditions are only possible when a cell is defined'):
+            trajectory.set_trajectory(**data)
+
+        data.update(
+            {
+                'cells': np.array([[[1.0, 0.0, 0.0], [0.0, 1.0, 0.0], [0.0, 0.0, 1.0]]]),
+                'pbc': None,
+            }
+        )
+        with pytest.warns(AiidaDeprecationWarning, match="When 'cells' is not None, the periodic"):
+            trajectory.set_trajectory(**data)
+
+        data.update(
+            {
+                'cells': np.array([[[1.0, 0.0, 0.0], [0.0, 1.0, 0.0], [0.0, 0.0, 1.0]]]),
+                'pbc': (True, False, False),
+            }
+        )
+        trajectory.set_trajectory(**data)
+        assert trajectory.get_step_structure(0).pbc == (True, False, False)