fix(pl): derive n_var_per_sample orders deterministically - #36
Open
idf-io wants to merge 2 commits into
Open
Conversation
The pre-commit hooks (pyupgrade --py310-plus, black --line-length=79) run only over the files a commit touches, and stats.py had not been touched since they were introduced. Any functional change to it therefore drags a thousand lines of reformatting along with it; doing the reformat on its own first keeps the change that follows readable. No behaviour change: black's layout, plus pyupgrade rewriting `from typing import Sequence` to `from collections.abc import Sequence`. One edit is not cosmetic, and the import rewrite forces it. The signature `color_scheme: str | dict | Sequence | Colormap | callable | None` used the bare `callable` builtin. While `Sequence` came from `typing`, the chain evaluated to a `typing.Union`, whose `_type_check` accepts anything callable, so the mistake was invisible. As `collections.abc.Sequence` it is a PEP 604 `types.UnionType`, which demands types and raises TypeError at import. It is now annotated `Callable`, which is what it always meant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`n_var_per_sample()`, and with it `n_peptides_per_sample()` and `n_proteins_per_sample()`, took every order it was not given from the position of rows in the AnnData object. A fractionation annotated F1..F10 plotted as F1, F10, F2 or in whatever sequence the reader's pivot happened to produce, and the only way to change it was to reorder the object. Nothing errored; the series was simply drawn out of order, and a profile read left to right was then wrong. Orders are now derived by one rule, `pl/_utils.py:resolve_default_order()`: the category order when the annotation is a Categorical, otherwise the values sorted lexicographically on their string form. It governs the sample axis, the `group_by` bars, the `order_by` blocks, and the samples within each block. A user fixes the order once, by storing the annotation as an ordered Categorical, instead of relying on how the file was read. The helper is shared so the remaining `pl` functions can adopt it as they are migrated. Labels come from `adata.obs["sample_id"]` rather than `adata.obs_names`. The drawn strings are unchanged — `check_proteodata()` enforces that the column and the index are identical — but an AnnData axis index is a plain string index and cannot carry a category order, so the column is the only source that can. This also removes two merges on the obs index: the counts frame and its grouping column are now built positionally from `.obs`. Three further defects in the same code path: - `order` appended the values it did not list instead of excluding them, which contradicted its documented subsetting semantics and made a subset impossible. It now subsets, and drops the excluded rows from the printed statistics as well as from the axis. Its values are validated against `adata.obs['sample_id']`. - `ascending` was silently ignored when combined with `order_by` — no sort, no warning. It now sorts the samples within each block. It remains ignored, with the existing warning, under `order` and `group_by`. - samples with a missing `order_by` value were labelled from `str(nan)`. They now form a trailing `NA` block, which `order` excludes like any other group. **This changes existing figures.** A plot whose bars followed `.obs_names` will now be sorted. The two tutorial notebooks that call these functions keep their stale committed outputs and will be re-run separately. 24 tests in tests/pl/test_n_var_per_sample.py — the first tests the package has for `pl` — covering each path twice, once with a categorical annotation and once without, plus the precedence of `order` and `ascending` over the default rule. Full suite: 473 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![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: 5f3a0df590
ℹ️ 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".
Comment on lines
1407
to
1409
| counts[group_key] = ( | ||
| counts[group_key].astype(str) | ||
| counts[group_key].astype(object).fillna(_NA_BLOCK_LABEL).astype(str) | ||
| ) |
There was a problem hiding this comment.
When an order_by column contains both a literal "NA" group and missing values, _n_var_resolve_obs_ordering() renames the missing-value block to "NA_", but this code still fills those rows with "NA". Consequently, the trailing bars labeled "NA_" receive the real "NA" group's color under both the default cycle and a user-supplied mapping, misrepresenting their group membership; carry the resolved collision-safe label into this fill operation.
AGENTS.md reference: AGENTS.md:L389-L390
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.
Purpose
n_var_per_sample()— and with itn_peptides_per_sample()andn_proteins_per_sample()— took every order it was not explicitly given from the position of rows in the AnnData object. A fractionation annotated F1..F10 plotted in whatever sequenceread.diann's pivot produced, and the only way to change it was to reorder the object. Nothing errored; the series was simply drawn out of order, and a profile read left to right was then wrong.This makes the function conform to the two
plstandards added inf86485d(AGENTS.md → Deterministic Ordering inpl, Axis Labels Come from ID Columns, Never from the Index).Key changes
proteopy/pl/_utils.py: resolve_default_order()— category order when the annotation is a Categorical, otherwise the values sorted lexicographically on their string form. It governs the sample axis, thegroup_bybars, theorder_byblocks, and the samples within each block. Users fix the order once by storing the annotation as an ordered Categorical. The helper is shared so the remainingplfunctions can adopt it as they are migrated.adata.obs["sample_id"], notadata.obs_names. The drawn strings are unchanged —check_proteodata()enforces that column and index are identical — but an AnnData axis index cannot carry a category order, so the column is the only source that can. Two merges on the obs index disappear with it: the counts frame and its grouping column are now built positionally from.obs.ordersubsets, as its documented semantics always claimed, instead of appending the values it did not list. Excluded rows leave the printed statistics as well as the axis. Values are validated againstadata.obs['sample_id'].ascending+order_byworks. It was silently ignored — no sort, no warning. It now sorts samples within each block. Still ignored, with the existing warning, underorderandgroup_by.order_byvalues form a trailingNAblock instead of being labelledstr(nan);orderexcludes it like any other group.Breaking
Existing figures change. A plot whose bars followed
.obs_nameswill now be sorted.HISTORY.mdrecords this under[Unreleased] → Changed. The two tutorial notebooks that call these functions keep their stale committed outputs and will be re-run separately.Commits
8f16bf5style — black/pyupgrade overstats.py, which the hooks had never touched, so the functional diff stays readable. Behaviour-neutral except one forced fix:color_scheme: ... | callable | Noneused the bare builtin. Undertyping.Sequencethe chain became atyping.Union, whose_type_checkaccepts any callable, hiding it; ascollections.abc.Sequenceit is a PEP 604types.UnionTypeand raisesTypeErrorat import. AnnotatedCallable.5f3a0dffix — everything above.Tests
tests/pl/test_n_var_per_sample.py, 24 tests — the package's firstpltests. Each path is covered twice, once with a categorical annotation and once without, plus the precedence oforderandascendingover the default rule.🤖 Generated with Claude Code