diff --git a/package/AUTHORS b/package/AUTHORS index 3d87a0651c..3a4ac88f42 100644 --- a/package/AUTHORS +++ b/package/AUTHORS @@ -286,6 +286,7 @@ Chronological list of authors - Aryaman Chaudhri - Francesco Siciliani - Akshit Boora + - Dhrub Chatterjee External code diff --git a/package/CHANGELOG b/package/CHANGELOG index 72b880c821..adc9f5c211 100644 --- a/package/CHANGELOG +++ b/package/CHANGELOG @@ -23,6 +23,10 @@ The rules for this file: * 2.11.0 Fixes + * Setting topology attributes (such as atom names, residue IDs, charges) + now validates that assigned values match the attribute's expected data type + and raises a clear TypeError rather than failing with confusing internal + errors or silently coercing invalid types (Issue #2764) * Fixed PBC handling in triclinic boxes in `_triclinic_pbc()` (used by `distances.apply_PBC()`) and `lib.nsgrid.NSGrid` (used by `capped_distance` and `self_capped_distance`): diff --git a/package/MDAnalysis/core/topologyattrs.py b/package/MDAnalysis/core/topologyattrs.py index 6ad9dc5198..a86651738e 100644 --- a/package/MDAnalysis/core/topologyattrs.py +++ b/package/MDAnalysis/core/topologyattrs.py @@ -85,6 +85,92 @@ from .. import _TOPOLOGY_ATTRS, _TOPOLOGY_TRANSPLANTS, _TOPOLOGY_ATTRNAMES +def _check_dtype(attr, group, values): + """Check that values match the expected dtype of the TopologyAttr.""" + is_component = isinstance(group, ComponentBase) + is_iter = iterable(values) + attr_name = attr.singular if is_component else attr.attrname + + if ( + isinstance(attr, _StringInternerMixin) + or getattr(attr, "dtype", None) == "U1" + ): + kind = "str" + type_desc = "a string" + plural_desc = "strings" + else: + dtype = getattr(attr, "dtype", None) + if dtype is None: + return + if np.issubdtype(dtype, np.integer): + kind = "int" + type_desc = "an integer" + plural_desc = "integers" + elif np.issubdtype(dtype, np.floating): + kind = "float" + type_desc = "a number" + plural_desc = "numbers" + elif np.issubdtype(dtype, np.bool_): + kind = "bool" + type_desc = "a boolean" + plural_desc = "booleans" + else: + return + + if is_iter: + if isinstance(values, np.ndarray) and values.dtype != object: + if kind == "str": + valid = np.issubdtype(values.dtype, np.str_) + elif kind == "int": + valid = np.issubdtype( + values.dtype, np.integer + ) and not np.issubdtype(values.dtype, np.bool_) + elif kind == "float": + valid = np.issubdtype( + values.dtype, np.number + ) and not np.issubdtype(values.dtype, np.bool_) + else: # bool + valid = np.issubdtype(values.dtype, np.bool_) + else: + if kind == "str": + valid = all(isinstance(v, str) for v in values) + elif kind == "int": + valid = all( + isinstance(v, (int, np.integer)) + and not isinstance(v, (bool, np.bool_)) + for v in values + ) + elif kind == "float": + valid = all( + isinstance(v, (int, float, np.number)) + and not isinstance(v, (bool, np.bool_)) + for v in values + ) + else: # bool + valid = all(isinstance(v, (bool, np.bool_)) for v in values) + + if not valid: + raise TypeError( + f"All values for {attr_name} must be {plural_desc}" + ) + else: + if kind == "str": + valid = isinstance(values, str) + elif kind == "int": + valid = isinstance(values, (int, np.integer)) and not isinstance( + values, (bool, np.bool_) + ) + elif kind == "float": + valid = isinstance( + values, (int, float, np.number) + ) and not isinstance(values, (bool, np.bool_)) + else: # bool + valid = isinstance(values, (bool, np.bool_)) + + if not valid: + raise TypeError(f"{attr_name} must be {type_desc}") + + def _check_length(func): """Wrapper which checks the length of inputs to set_X @@ -151,6 +237,7 @@ def wrapper(attr, group, values): lenvalues=val_len, ) ) + _check_dtype(attr, group, values) # if everything went OK, continue with the function return func(attr, group, values) @@ -1947,7 +2034,7 @@ def _gyration(recenteredpos, masses): recenteredpos = atomgroup.positions - com eig_vals = _gyration(recenteredpos, masses) else: - (atom_masks, compound_masks, n_compounds) = ( + atom_masks, compound_masks, n_compounds = ( atomgroup._split_by_compound_indices(compound) ) @@ -2408,7 +2495,7 @@ def dipole_vector( "ij,ij->j", recenteredpos, charges[:, np.newaxis] ) else: - (atom_masks, compound_masks, n_compounds) = ( + atom_masks, compound_masks, n_compounds = ( atomgroup._split_by_compound_indices(compound) ) @@ -2619,7 +2706,7 @@ def __quadrupole_tensor(recenteredpos, charges): recenteredpos = atomgroup.positions - ref quad_tensor = __quadrupole_tensor(recenteredpos, charges) else: - (atom_masks, compound_masks, n_compounds) = ( + atom_masks, compound_masks, n_compounds = ( atomgroup._split_by_compound_indices(compound) ) diff --git a/testsuite/MDAnalysisTests/core/test_topologyattrs.py b/testsuite/MDAnalysisTests/core/test_topologyattrs.py index 4d8b3349aa..41fe111bb2 100644 --- a/testsuite/MDAnalysisTests/core/test_topologyattrs.py +++ b/testsuite/MDAnalysisTests/core/test_topologyattrs.py @@ -20,9 +20,8 @@ # MDAnalysis: A Toolkit for the Analysis of Molecular Dynamics Simulations. # J. Comput. Chem. 32 (2011), 2319--2327, doi:10.1002/jcc.21787 # -"""Tests for MDAnalysis.core.topologyattrs objects. +"""Tests for MDAnalysis.core.topologyattrs objects.""" -""" import numpy as np from numpy.testing import ( @@ -125,8 +124,8 @@ def test_set_atoms_singular(self, attr): def test_set_atoms_plural(self, attr): # set len 2 Group to len 2 values dg = DummyGroup([3, 7]) - attr.set_atoms(dg, np.array([23, 504])) - assert_equal(attr.get_atoms(dg), np.array([23, 504])) + attr.set_atoms(dg, self.values[:2]) + assert_equal(attr.get_atoms(dg), self.values[:2]) def test_set_atoms_VE(self, attr): # set len 2 Group to wrong length values @@ -305,10 +304,8 @@ def test_set_residues_singular(self, attr): ) def test_set_residues_plural(self, attr): - attr.set_residues(DummyGroup([3, 0, 1]), np.array([23, 504, 2])) - assert_almost_equal( - attr.get_residues(DummyGroup([3, 0, 1])), np.array([23, 504, 2]) - ) + attr.set_residues(DummyGroup([3, 0, 1]), self.values[:3]) + assert_equal(attr.get_residues(DummyGroup([3, 0, 1])), self.values[:3]) def test_set_residues_VE(self, attr): dg = DummyGroup([3, 0, 1]) @@ -786,3 +783,153 @@ def test_coc_unwrap(self, u, compound): u.atoms.wrap coc = u.atoms[:2].center_of_charge(compound=compound, unwrap=True) assert_equal(coc.flatten(), [0, -0.5, 0]) + + +class TestDtypeChecks: + @pytest.fixture + def u(self): + universe = mda.Universe.empty( + n_atoms=6, + n_residues=3, + n_segments=2, + atom_resindex=[0, 0, 1, 1, 2, 2], + residue_segindex=[0, 0, 1], + ) + universe.add_TopologyAttr("names", ["C", "CA", "N", "O", "CB", "CG"]) + universe.add_TopologyAttr("resnames", ["ALA", "GLU", "VAL"]) + universe.add_TopologyAttr("segids", ["SEG1", "SEG2"]) + universe.add_TopologyAttr("resids", [1, 2, 3]) + universe.add_TopologyAttr("charges", [0.0, 0.0, 0.0, 0.0, 0.0, 0.0]) + universe.add_TopologyAttr( + "masses", [12.0, 12.0, 14.0, 16.0, 12.0, 12.0] + ) + universe.add_TopologyAttr( + "aromaticities", [False, False, False, False, False, False] + ) + return universe + + @pytest.mark.parametrize("invalid", [1, 2.5, True, None]) + def test_string_attr_atom_invalid(self, u, invalid): + with pytest.raises(TypeError, match="must be a string"): + u.atoms[0].name = invalid + + @pytest.mark.parametrize( + "invalid", [[1, 2, 3], ["C", 1, "O"], np.array([1, 2, 3]), 1] + ) + def test_string_attr_group_invalid(self, u, invalid): + with pytest.raises(TypeError, match="must be (a string|strings)"): + u.atoms[:3].names = invalid + + @pytest.mark.parametrize("invalid", [2.5, "1", True, None]) + def test_int_attr_residue_invalid(self, u, invalid): + with pytest.raises(TypeError, match="must be an integer"): + u.residues[0].resid = invalid + + @pytest.mark.parametrize( + "invalid", + [[1.5, 2.5], np.array([1.5, 2.5]), [1, "2"], [True, False], 2.5], + ) + def test_int_attr_group_invalid(self, u, invalid): + with pytest.raises(TypeError, match="must be (an integer|integers)"): + u.residues[:2].resids = invalid + + @pytest.mark.parametrize("invalid", ["positive", True, None]) + def test_float_attr_atom_invalid(self, u, invalid): + with pytest.raises(TypeError, match="must be a number"): + u.atoms[0].charge = invalid + + @pytest.mark.parametrize( + "invalid", + [ + ["a", "b", "c"], + [True, False, True], + np.array([True, False, True]), + "positive", + ], + ) + def test_float_attr_group_invalid(self, u, invalid): + with pytest.raises(TypeError, match="must be (a number|numbers)"): + u.atoms[:3].charges = invalid + + @pytest.mark.parametrize("invalid", [1, 0, "true", None]) + def test_bool_attr_atom_invalid(self, u, invalid): + with pytest.raises(TypeError, match="must be a boolean"): + u.atoms[0].aromaticity = invalid + + @pytest.mark.parametrize( + "invalid", + [[1, 0, 1], ["a", "b", "c"], np.array([1, 0, 1]), 1], + ) + def test_bool_attr_group_invalid(self, u, invalid): + with pytest.raises(TypeError, match="must be (a boolean|booleans)"): + u.atoms[:3].aromaticities = invalid + + def test_valid_assignments(self, u): + u.atoms[0].name = "N" + assert u.atoms[0].name == "N" + + u.atoms[:3].names = ["C1", "C2", "C3"] + assert list(u.atoms[:3].names) == ["C1", "C2", "C3"] + + u.atoms[:3].names = np.array(["C1", "C2", "C3"]) + assert list(u.atoms[:3].names) == ["C1", "C2", "C3"] + + u.atoms.names = "CA" + assert all(name == "CA" for name in u.atoms.names) + + u.residues[0].resid = 10 + assert u.residues[0].resid == 10 + + u.residues[:2].resids = [100, 200] + assert list(u.residues[:2].resids) == [100, 200] + + u.residues[:2].resids = np.array([100, 200]) + assert list(u.residues[:2].resids) == [100, 200] + + u.residues.resids = 1 + assert all(r == 1 for r in u.residues.resids) + + # ints should be allowed for float attributes + u.atoms[0].charge = 1 + assert u.atoms[0].charge == 1.0 + + u.atoms[:2].charges = [-0.5, 0.5] + assert_equal(u.atoms[:2].charges, [-0.5, 0.5]) + + u.atoms[:2].charges = np.array([-0.5, 0.5]) + assert_equal(u.atoms[:2].charges, [-0.5, 0.5]) + + u.atoms.charges = 0.0 + assert all(c == 0.0 for c in u.atoms.charges) + + u.atoms[0].aromaticity = True + assert u.atoms[0].aromaticity == True + + u.atoms[:3].aromaticities = [True, False, True] + assert list(u.atoms[:3].aromaticities) == [True, False, True] + + u.atoms[:3].aromaticities = np.array([True, False, True]) + assert list(u.atoms[:3].aromaticities) == [True, False, True] + + u.atoms.aromaticities = True + assert all(a == True for a in u.atoms.aromaticities) + + def test_unsupported_or_none_dtype_bypass(self, u): + from MDAnalysis.core.topologyattrs import _check_dtype, TopologyAttr + + class DNone(TopologyAttr): + attrname = "dn" + singular = "d" + + def __init__(self): + self.dtype = None + + class DObj(TopologyAttr): + attrname = "do" + singular = "d" + + def __init__(self): + self.dtype = object + + _check_dtype(DNone(), u.atoms, [1, 2]) + _check_dtype(DObj(), u.atoms, [1, 2])