Making max_seq_count actually truncate parsed MSAs#308
Open
Pana-TsK wants to merge 2 commits into
Open
Conversation
parse_a3m and parse_stockholm called MsaArray.truncate() without inplace=True and discarded the returned copy, so max_seq_count was a no-op and every sequence was kept. Pass inplace=True so the cap applies, and simplify MsaArray.truncate's row_slice handling (drop the never-raised ValueError and the redundant bounds clamp, which numpy/pandas already do). Add regression tests for both parsers and for truncate's int/slice paths.
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
1 participant
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.
Summary
Behavior change: Previously all parsed sequences were kept. The final MSA-rows feature was then bounded by the downstream max_rows (16384) subsampling, but the profile and deletion-mean features are computed on the full pre-subsample MSA (main_msa_redundant), so they were never bounded. This fix therefore changes those features for any chain whose source MSAs exceed their caps, and also changes which databases populate the max_rows budget. Chains with shallow MSAs (already under every cap) are unaffected.
parse_a3m()andparse_stockholm()calledMsaArray.truncate()withoutinplace=True, leading to Msa truncation being calculated, but set to return a new MsaArray object, which was then discarded.Additionally, The method also constructed a ValueError without ever raise-ing it (the same discarded-expression bug), so a wrong-typed input passed silently instead of erroring, which I removed. The line
row_slice = slice(min(row_slice.stop, self.__len__()))is redundant. It attempts to clamp the count so the caller can never ask for more rows than exist, but numpy and pandas already clamp out-of-range slices, indexing past the end just returns everything, without error. the regression test test_truncate_over_length_keeps_all_rows covers this.Changes
parse_a3m()andparse_stockholm()now useinplace=True, so the methods return truncated MSAs as intended. Additionally, the mentioned valueError has been deleted (it would never fire), the linerow_slice = slice(min(row_slice.stop, self.__len__()))has also been deleted.Testing
AI-generated regression tests (simple, easy to check & reason about), have been written, to show that this issue was real and now has been resolved.