Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR closes #286, fixing a bug where out-of-bounds
molidxsearches silently returned the last molecule instead of raising aKeyError. The root cause was inIDIndexEngine::searchMolIdx(): unlikesearchIdx<T>(used for atoms, residues, etc.), it had no single-value fast path and instead fell through to amatch()call that usedslice.begin(count, true)withauto_fix=true, silently clamping any out-of-bounds index to the last valid position. The fix adds a_is_single_valuefast path tosearchMolIdxthat reads the raw index directly fromRangeValue::start(bypassing the_to_single_valuehelper, which corrupts negative indices by mapping them againstINT_MAX) and applies strict Python-style bounds checking. Negative indices within range (e.g. -1) continue to work correctly, while out-of-bounds values in either direction now return no match, consistent with residx and atomidx behaviour. A secondary bug inMolIdx::map(const Molecules &)where the loop counter i was never decremented is also fixed. A regression test is included.Debugged with assistance from Claude Code Opus 4.6.
@chryswoods: Just want to address you concerns about different container types and use case. In the C++ layer we have
MolIdx::map(const Molecules&), whereMoleculesis aQHash<MolNum, ViewsOfMol>, so has no stable iteration order. I think this isn't an issue for user-facing code, since it goes through another pathway, but good to check. The comment referencingstatic map()in the issue thread, this pathway isn't touched by this fix, so was a red-herring, i.e. that's used for ranges and comparisons only. For the additional bug fix: I don't think this was ever an issue since this low-level overload probably wasn't used by any callers.develinto this branch before issuing this pull request (e.g. by runninggit pull origin devel): [y]