feat(pp): add summarize_peptides_by_neighbourhood_union - #34
Merged
Conversation
Test-first specification for a new preprocessing function that reimplements CCprofiler's summarizeAlternativePeptideSequences(topN=1) and the proteinQuantification(topN, keep_less) selection it delegates to. The existing summarize_overlapping_peptides is untouched. The function groups peptides by positional overlap in the protein sequence rather than by substring containment, and selects the most abundant member of each group instead of aggregating them. Peptide positions are resolved from a FASTA inside the same call. 115 tests over 14 topics: FASTA header parsing, position annotation, stripped-sequence alphabet validation, unknown proteins, unlocated peptides, grouping semantics, selection, tie breaking, identifier naming, provenance, .var column guards, missing values, top_n and keep_less, output ordering, and the API contract. Every expected value is derived by hand from a small synthetic protein with twenty distinct residues, so no expectation is produced by the code under test. Several tests exist to stop faithful-but-surprising behaviour being "corrected" later: - an A-B-C-D overlap chain yields three groups, not one; transitive closure would select one peptide where the reference selects three - peptides whose position cannot be resolved share the empty label and collapse into a single group per protein - a peptide with any missing sample has a NaN total and ranks last, reproducing R's sum(na.rm = FALSE) with rank(na.last = TRUE) - fill_na=0 is shown to select a different peptide than the faithful path, so it cannot quietly become a default Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Pure restructuring of the summarize_peptides_by_neighbourhood_union tests. The sixteen topic classes become one class named after the function under test; each former class survives as a header comment carrying its name and its docstring, so the topic groupings and the measured evidence recorded in them are unchanged. Two tests were renamed. TestUnknownProtein and TestUnlocatedPeptides each defined test_error_names_the_available_modes; in a single class the second definition would have shadowed the first and silently dropped a test. They are now prefixed with the case they cover. Verified by comparing the AST of every test body before and after: 115 methods, identical names, identical bodies. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reimplements CCprofiler's summarizeAlternativePeptideSequences(topN=1)
and the proteinQuantification(topN, keep_less) selection it delegates
to. summarize_overlapping_peptides is untouched.
Peptides are grouped by the union of their positional neighbourhoods in
the protein sequence rather than by substring containment, and the most
abundant member of each group is selected rather than aggregated.
Positions are resolved from a FASTA inside the same call, so no
separate annotation step is needed, and no modification-summarisation
step either: two peptidoforms of one stripped sequence share an
interval and therefore a group.
Behaviour worth knowing:
- Missing values follow CCprofiler with no parameter to choose
otherwise. A peptide with any missing sample has a NaN total and
ranks last; it is deprioritised, not removed, so one with no complete
competitor still wins its group and passes through untouched.
- mod_regex is self-checking. Whatever remains after stripping must be
amino acids, so an uncovered mass shift, bracket tag or lowercase
marker is an error instead of a silently unlocatable peptide. The
default alphabet is the IUPAC set, which admits selenocysteine and
the ambiguity codes.
- on_unknown_protein and on_unlocated_peptide each take
{'raise', 'skip', 'keep'} and default to 'raise'. 'keep' is the
CCprofiler behaviour, giving NaN positions so the peptides share the
empty label and collapse into one group per protein.
- Ties are broken by tie_break_key, not by input order, so the result
does not depend on how the input table was sorted. The default places
'(' and '[' after the letters.
- Output .var is reduced to peptide_id, protein_id, peptide_start,
peptide_end, the provenance column and n_grouped. The survivor's
other annotations are one member's, not the group's, so carrying them
would invite them to be read as representative. Layers are dropped
for the same reason. Columns the function writes must be absent from
the input, so a second call raises rather than overwriting.
116 tests pass; the full suite is green at 436.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Lets a caller carry chosen .var annotations through the summarisation instead of losing them, following the precedent set by summarize_modifications. Values are aggregated across the whole group, unique values joined by ';', rather than taken from the surviving row. Taking the survivor's value would reintroduce exactly the confusion the column pruning exists to prevent: one member's metadata read as the group's. A column that is identical across members collapses to that single value, which is the usual case for protein-level annotations such as a gene symbol. A name the function already writes is rejected before the column-exists check, since it is a contract error whether or not the caller happens to have such a column. 7 tests added, 123 total; full suite green at 444. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Docstring - Module header now states the algorithm in notation: membership N(x) as the interval-overlap neighbourhood, the label L(x) as the one-hop union over neighbourhoods containing x, the group G(x) as equal labels, and the ranking key used to select within a group. - A worked example runs the same five peptides used in the Examples section: a four-peptide chain in which only adjacent pairs overlap, plus one peptide lying strictly inside another. It shows N, L and G for each and why the result is three groups rather than one. - Missing values get their own section covering both places they act: a missing total sorts last in the ranking, and at top_n > 1 the sum propagates. The all-members-missing case is spelled out for each. - Links the CCprofiler branch this reimplements: https://github.com/CCprofiler/CCprofiler/tree/proteoformLocationMapping Behaviour - Input must be peptide-level proteodata. Previously only the two columns were checked, so a var_names/peptide_id mismatch or a multi-mapped peptide reached the algorithm. Skipped for a zero-peptide object, which is_proteodata reports as level-less and which is a legitimate if degenerate input. - Sparse .X is densified rather than round-tripped. The algorithm ranks and reorders whole columns and gains nothing from sparsity, and a peptide intensity matrix is not meaningfully sparse: an absent measurement is missing, not zero. Output .X is always dense. 128 tests; full suite green at 449; the docstring example runs as a doctest. The mouse-tissue reference result is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follows the structure set out in the working copy: no module docstring, everything on the function, and the citation in its own section. - The algorithm is now three lines of notation plus two short paragraphs on what makes N and L unusual, in place of the previous section-by-section walkthrough. The full N/L/G derivation lives in test_overlap_chain_yields_three_groups_not_one, which is where it is actually checked. - Missing values reduced to one paragraph covering both the ranking and the summation, including the all-missing case. - The example keeps the same five peptides but drops the intermediate commentary and two of the five assertions. - References section in numpydoc form, cited as [1] from the summary, matching the other proteopy functions. Authors taken from the package DESCRIPTION rather than guessed. - See Also points at summarize_overlapping_peptides, which is where the containment-and-aggregate contrast now lives. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The function and its default ordering key were importable but appeared nowhere in the Sphinx site, so neither had a generated page and the :func:`letters_first_key` cross-reference in the parameter description had nothing to resolve to. - docs/sphinx/source/api/pp.rst: both added under Quantification, beside the other summarize_* functions. - HISTORY.md: an Added entry under [Unreleased], covering the grouping rule, selection versus aggregation, the missing-value policy, the deterministic tie-break, the two unresolved-position policies and the .var reduction with keep_var_cols. Verified by building the site: both pages are read, imported and parsed without warnings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It is the default value of `tie_break_key`, not something a caller needs to reach for: anyone overriding the tie-break supplies their own key, and anyone accepting the default never names it. Dropping it from `pr.pp` keeps the public surface to the one function. - removed from `proteopy/pp/__init__.py` and from the API docs - the parameter description now states what the default ordering does rather than cross-referencing a symbol that no longer resolves, and the changelog entry does the same The function itself is unchanged and still reachable by module path. Full suite green at 449; the docstring example still runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Documentation build overview
6 files changed ·
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8f8bf2bb4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # columns and gains nothing from sparsity, and a matrix of peptide | ||
| # intensities is not meaningfully sparse -- an absent measurement is | ||
| # missing, not zero. | ||
| X = X.toarray() if hasattr(X, "toarray") else np.asarray(X) |
There was a problem hiding this comment.
When adata.X is sparse, this unconditional conversion loses the input format and _rebuild_adata consequently returns or installs a dense matrix. Besides breaking callers that rely on sparse output, this can substantially increase memory and serialized dataset size; record sparse.issparse(adata.X) and convert X_new back to the original format or CSR before rebuilding.
AGENTS.md reference: AGENTS.md:L71-L77
Useful? React with 👍 / 👎.
Comment on lines
+750
to
+752
| columns=[ | ||
| peptide_col, | ||
| protein_col, |
There was a problem hiding this comment.
When a valid ProteoData object also contains aliases and the caller sets peptide_col or protein_col, var_new is built only with those aliases, dropping the canonical peptide_id/protein_id columns. The returned object then fails is_proteodata classification and downstream ProteoPy functions cannot rely on the required ID schema; either keep synchronized canonical columns in the output or reject noncanonical column names.
AGENTS.md reference: AGENTS.md:L59-L61
Useful? React with 👍 / 👎.
Comment on lines
+743
to
+746
| if columns: | ||
| X_new = np.column_stack(columns) | ||
| else: | ||
| X_new = np.empty((adata.n_obs, 0), dtype=float) |
There was a problem hiding this comment.
If every group is filtered out—for example, a singleton with top_n=2, keep_less=False, or all peptides skipped by an unresolved-position policy—this path returns an AnnData with zero variables. is_proteodata reports (False, None) for that result, while the final check_proteodata call silently ignores the false tuple, so callers receive an object that no longer satisfies the public ProteoData contract; explicitly raise for this outcome or establish a validator-supported empty representation before returning.
AGENTS.md reference: AGENTS.md:L37-L42
Useful? React with 👍 / 👎.
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.
Adds
pr.pp.summarize_peptides_by_neighbourhood_union(), which collapses peptides that overlap in the protein sequence and keeps the most abundant member of each group. Peptide positions are resolved from a FASTA in the same call, so no separate annotation step is needed.A reimplementation of CCprofiler's
summarizeAlternativePeptideSequences(topN = 1)(branchproteoformLocationMapping).summarize_overlapping_peptidesis untouched.Why a separate function
summarize_overlapping_peptidesPositional overlap sees pairs that containment cannot: two peptides can overlap in the protein while neither contains the other. It also needs no separate modification-summarisation step, since two peptidoforms of one stripped sequence share an interval and therefore a group.
Behaviour worth a look during review
top_n > 1the sum propagates missingness.tie_break_key, not by input row order, so the result does not depend on how the input table happened to be sorted. The default places non-letters after letters, favouring the unmodified form of an identifier.mod_regexis self-checking. Whatever remains after stripping must be amino acids, so an uncovered mass shift, bracket tag or lowercase marker raises instead of silently producing an unlocatable peptide. The default alphabet is the IUPAC set, which admits selenocysteine and the ambiguity codes.on_unknown_proteinandon_unlocated_peptideeach take{"raise", "skip", "keep"}and default to"raise"..varis reduced to the peptide-level proteodata columns plus the function's own output. The surviving row's other annotations describe one member rather than the group, so carrying them would invite them to be read as representative;keep_var_colscarries chosen columns through, aggregated across the group..Xis densified; output.Xis always dense.Tests
128 tests, written before the implementation, with every expected value derived by hand from a synthetic twenty-residue protein. Several exist specifically to stop faithful-but-surprising behaviour being "corrected" later — the one-hop union rather than a transitive closure, the empty-label collapse of unlocated peptides, and the missing-value ranking. The docstring example runs as a doctest. Full suite green at 449.
Also
docs/sphinx/source/api/pp.rst: listed under Quantification.HISTORY.md:Addedentry under[Unreleased].Unrelated, noted in passing: the Sphinx build currently aborts on
mainwith aSEVERE: Unexpected section titlefromsphinx_autodoc_typehints, before any HTML is written. It reproduces without this branch and is not addressed here.