diff --git a/corelib/src/libs/SireMol/molid.cpp b/corelib/src/libs/SireMol/molid.cpp index 6cb608d07..ddbb4bed4 100644 --- a/corelib/src/libs/SireMol/molid.cpp +++ b/corelib/src/libs/SireMol/molid.cpp @@ -266,6 +266,8 @@ QList MolIdx::map(const Molecules &molecules) const molnums.append(it.key()); break; } + + --i; } BOOST_ASSERT(not molnums.isEmpty()); diff --git a/corelib/src/libs/SireSearch/idengine.cpp b/corelib/src/libs/SireSearch/idengine.cpp index 73992526c..079f48830 100644 --- a/corelib/src/libs/SireSearch/idengine.cpp +++ b/corelib/src/libs/SireSearch/idengine.cpp @@ -31,8 +31,8 @@ #include "SireBase/booleanproperty.h" #include "SireBase/parallel.h" -#include "SireMol/atomelements.h" #include "SireMol/atomcoords.h" +#include "SireMol/atomelements.h" #include "SireMol/core.h" #include "SireMol/iswater.h" @@ -1155,9 +1155,63 @@ SelectResult IDIndexEngine::searchMolIdx(const SelectResult &mols, const SelectR { QList matches; - int idx = 0; int count = context.listCount(); + if (_is_single_value(this->vals)) + { + // For a single index value, apply Python-style negative-index mapping + // and do a strict bounds check. Out-of-bounds returns no match, + // consistent with residx/atomidx behaviour. + // We read the raw start value from RangeValue directly, bypassing the + // _to_single_value helper which maps against INT_MAX and corrupts + // negative indices. + auto rv = boost::get(this->vals[0]); + + if (not rv.start) + return SelectResult(matches); + + int v = *rv.start; + + if (v < 0) + v = count + v; + + if (v < 0 or v >= count) + return SelectResult(matches); + + int idx = 0; + + for (const auto &mol : context) + { + if (idx == v) + { + if (&mols == &context) + { + matches.append(mol->molecule()); + } + else + { + const auto molnum = mol->data().number(); + + for (const auto &m : mols) + { + if (m->data().number() == molnum) + { + matches.append(m->molecule()); + break; + } + } + } + break; + } + + idx += 1; + } + + return SelectResult(matches); + } + + int idx = 0; + for (const auto &mol : context) { if (this->match(idx, count)) diff --git a/doc/source/changelog.rst b/doc/source/changelog.rst index 472b973aa..7b9ca156b 100644 --- a/doc/source/changelog.rst +++ b/doc/source/changelog.rst @@ -58,6 +58,10 @@ organisation on `GitHub `__. * Map the end-state ``element`` property when performing hydrogen mass repartitioning on perturbable molecules. +* Fixed out-of-bounds ``molidx`` searches silently returning the last molecule instead of + raising a ``KeyError``. Out-of-bounds positive and negative single-index values now + behave consistently with ``residx`` and ``atomidx``. + `2025.4.0 `__ - February 2026 --------------------------------------------------------------------------------------------- diff --git a/ruff.toml b/ruff.toml new file mode 100644 index 000000000..cd7595702 --- /dev/null +++ b/ruff.toml @@ -0,0 +1,5 @@ +[lint] +ignore = ["E402"] + +[lint.per-file-ignores] +"tests/**" = ["F841"] diff --git a/tests/mol/test_complex_indexing.py b/tests/mol/test_complex_indexing.py index e87134b0a..159e912b8 100644 --- a/tests/mol/test_complex_indexing.py +++ b/tests/mol/test_complex_indexing.py @@ -442,7 +442,7 @@ def test_search_terms(ala_mols): check_mass = mols[0][0].mass().value() atoms = mols[0][ - f"atom mass >= {check_mass-0.001} and atom mass <= {check_mass+0.001}" + f"atom mass >= {check_mass - 0.001} and atom mass <= {check_mass + 0.001}" ] assert len(atoms) > 0 @@ -460,7 +460,7 @@ def test_search_terms(ala_mols): check_charge = mols[0][1].charge().value() atoms = mols[0][ - f"atom charge >= {check_charge-0.001} and atom charge <= {check_charge+0.001}" + f"atom charge >= {check_charge - 0.001} and atom charge <= {check_charge + 0.001}" ] assert len(atoms) > 0 @@ -527,8 +527,6 @@ def test_in_searches(ala_mols): def test_with_searches(ala_mols): mols = ala_mols - import sire as sr - for mol in mols["molecules with count(atoms) >= 3"]: assert mol.num_atoms() >= 3 @@ -593,3 +591,29 @@ def test_count_searches(ala_mols): mol = mols["molecules with count(residues) == 3"] assert mol.num_residues() == 3 + + +def test_oob_molidx(ala_mols): + """ + Regression test for issue #286: out-of-bounds molidx should raise KeyError, + not silently return the last molecule. + """ + mols = ala_mols + + n = mols.num_molecules() + + # Valid boundary indices should work + assert mols["molidx 0"] == mols[0] + assert mols[f"molidx {n - 1}"] == mols[-1] + assert mols["molidx -1"] == mols[-1] + + # Out-of-bounds positive index must raise KeyError + with pytest.raises(KeyError): + mols[f"molidx {n}"] + + with pytest.raises(KeyError): + mols["molidx 10000"] + + # Out-of-bounds negative index must raise KeyError + with pytest.raises(KeyError): + mols[f"molidx -{n + 1}"]