fix(datasets): preserve zeros and propagate NaN in williams_2018 - #33
Merged
Conversation
Pure formatting, no behaviour change. Split out so the missing-value fix in the following commit can be reviewed without reformatting noise. These files were not black-clean on main, and the repository's pre-commit runs black over changed files, so a PR touching them has to absorb this either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Zeros were being coerced to missing values. A zero in this dataset is a
measurement -- the peptide was looked for and its intensity was zero --
and is not interchangeable with "not measured". 13,547 of the 1,307,600
cells are genuine zeros and were all being discarded.
Two related problems in the charge-state summation, both hidden by the
coercion above:
- pandas' default min_count=0 meant a group whose charge states were
ALL missing summed to 0.0, inventing 3,324 measurements. Those
zeros were then converted straight back to NaN, so fixing this
alone is a no-op -- but it is wrong on its own terms.
- the same sum skipped NaN inside a PARTIALLY measured group, so
[NaN, 5000] summed to 5000, reporting a partial total as complete.
260 cells. min_count does not govern this; it decides the
all-missing case only, and pandas has no skipna on GroupBy.sum, so
the fix masks on the count.
A peptide is now quantified in a sample only if every one of its charge
states was quantified there.
Adds a `zero_to_na` parameter (default False) to the dataset loader and
the download wrapper, per the convention in AGENTS.md, mutually
exclusive with `fill_na` as in `pl.peptide_intensities`. It governs zero
semantics only: a partially measured group stays NaN under
`zero_to_na=True`, because it was never measured completely rather than
because its total happened to be zero.
`.X` and the downloaded intensities file change, and their test hashes
change with them. The var and sample annotation hashes are unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Documentation build overview
60 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: 5de06e497a
ℹ️ 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".
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.
What
williams_2018()coerced every zero in.Xtonp.nan. A zero in this dataset is a measurement —the peptide was looked for and its intensity was zero — and is not interchangeable with "not
measured". 13,547 of the 1,307,600 cells are genuine zeros and were all being discarded.
Removing that coercion exposed two related problems in the charge-state summation, both of which it
had been hiding:
min_count=0(pandas default)0.0, inventing a measurementNaNskipped in a partially measured group[NaN, 5000]sums to5000, reporting a partial total as completeThe invented zeros were previously converted straight back to
NaNby the coercion, so fixing thatone alone is a no-op — but it is wrong on its own terms, and the partial-total case is not.
min_countdoes not address the second: it governs the all-missing case only, and pandas has noskipnaonGroupBy.sum, so the fix masks on the count.A peptide is now quantified in a sample only if every one of its charge states was quantified there.
zero_to_naSince zeros now survive, both
datasets.williams_2018()anddownload.williams_2018()gainzero_to_na: bool = False, per the convention inAGENTS.md. Mutually exclusive withfill_na(raises
ValueError), matchingpl.peptide_intensities.It governs zero semantics only. A partially measured group stays
NaNunderzero_to_na=True,because it was never measured completely — not because its total happened to be zero.
Resulting
.XBreaking change
.Xand the file written bydownload.williams_2018()both change, and their test hashes changewith them. The var and sample annotation hashes are unchanged — the fix touches
.Xonly.Commits
style:— black over the four files. They were not black-clean onmain, and pre-commitruns black over changed files, so a PR touching them has to absorb it. Split out so the logic is
reviewable on its own.
fix:— the change.HISTORY.mdupdated under[Unreleased]. Full suite passes locally (Python 3.10, pandas 2.3.3).🤖 Generated with Claude Code