From 2079f43056849cc00c26eb7e4415ca7ef72a6078 Mon Sep 17 00:00:00 2001 From: Lester Hedges Date: Fri, 17 Apr 2026 14:45:46 +0100 Subject: [PATCH 1/2] Fix out-of-bounds molidx searches silently returning the last molecule. --- corelib/src/libs/SireMol/molid.cpp | 2 + corelib/src/libs/SireSearch/idengine.cpp | 58 +++++++++++++++++++++++- doc/source/changelog.rst | 4 ++ tests/mol/test_complex_indexing.py | 32 +++++++++++-- 4 files changed, 90 insertions(+), 6 deletions(-) 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/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}"] From d789fdd98780d219611ee63e30a36125447ada9c Mon Sep 17 00:00:00 2001 From: Lester Hedges Date: Fri, 17 Apr 2026 16:24:25 +0100 Subject: [PATCH 2/2] Add missing ruff.toml file. [ci skip] --- ruff.toml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 ruff.toml 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"]