design_matrix(fillna=None) turns a stored NULL pg intensity into 0.0; the in-memory and out-of-core pivots disagree
- Dominant language
- Python
- Stars
- 26
- Forks
- 8
- Avg merge
- 5h 13m
- Merged PRs (30d)
- 27
Description
`Dataset.design_matrix(level="protein", fillna=None)` is documented to preserve missingness, but a `pg` row whose `label` is non-null and whose `intensity` is **NULL** comes out of it as `0.0`.
Checked against `main` @ `b361f204` (= `v1.1.2`), with pandas 3.0.1 and duckdb 1.5.5.
### What happens
`_abundance_sql("protein")` correctly preserves the NULL — it selects `p.intensity` and filters only on label match and `is_decoy` (`qpx/dataset.py:546-552`), so the row reaches pandas as `NaN`.
The in-memory pivot then aggregates it away (`qpx/dataset.py:677`):
```python
matrix = df.pivot_table(index="sample_accession", columns="feature_id",
values="intensity", aggfunc="sum")
...
if fillna is not None:
matrix = matrix.fillna(fillna)
```
pandas' `aggfunc="sum"` uses `min_count=0`, so a group whose only value is `NaN` aggregates to `0.0`. That happens before `fillna` is consulted, and the check at `:681` cannot tell that synthesized zero from a real one — so `fillna=None` cannot preserve the missingness the docstring promises at `:637-638`:
```
fillna : float | None
Value to fill missing cells. Use *None* to keep NaN.
```
Note this is not just an ordering problem: reordering the `fillna` check alone would not fix `fillna=None`, because the null is already gone.
### The two documented paths disagree
The out-of-core path (`output_path=`) uses DuckDB `PIVOT ... USING SUM(intensity)`, and DuckDB returns `NULL` for an all-NULL group. So the same input gives `0.0` in memory and `NULL` on disk. `fillna` being ignored under `output_path` is documented (`:638`); this difference in aggregation semantics is not.
```
pandas 3.0.1 : all-NaN group, aggfunc="sum" -> 0.0 (isna: False)
duckdb 1.5.5 : SUM over an all-NULL group -> NULL
```
### Why it matters
A NULL `pg.intensity` beside a populated label means "there is evidence for this group here, but it was not quantified for this label". Turning it into `0.0` presents it as a measurement of zero, which then participates in ratios, log transforms and normalisation as data. It is the same failure mode as the `observed_mz = 0.0` sentinel removed in #244 — a sentinel that looks like a real value, where NULL would have propagated as "unknown".
The `fillna=0.0` default is not in question: asking for zeros and getting zeros is fine. The problem is that `fillna=None` cannot opt out.
### Reachability
This is not hypothetical. `openms_consensus` emits exactly this shape — `_rollup` returns `None` when a group has no unique-peptide signal in a unit+label, and that value is written with the populated label (`qpx/converters/openms_consensus/pg_adapter.py:345`). `PgWriter` deliberately preserves it (`qpx/writers/pg.py:43-52`).
### Test coverage
`tests/integration/test_dataset.py:478` exercises `fillna=None`, but only for an **absent** sample/protein combination — a coordinate missing from the pivot entirely, which does stay `NaN`. No test covers a present row carrying a NULL intensity, which is the case that collapses.
### Repro
```python
import pandas as pd, numpy as np
df = pd.DataFrame({"sample_accession": ["S1"], "feature_id": ["P1"], "intensity": [np.nan]})
m = df.pivot_table(index="sample_accession", columns="feature_id",
values="intensity", aggfunc="sum")
print(repr(m.loc["S1", "P1"]), pd.isna(m.loc["S1", "P1"])) # 0.0 False
```
End to end, a single pg row with `label='LFQ'`, `intensity=NULL` and a matching `run.samples[]` entry yields `design_matrix(level="protein", fillna=None).loc["S1", "P1"] == 0.0`, while the same base query under the DuckDB pivot yields `NULL`.
### Possible directions
Not a patch proposal, just the options I can see: aggregate with `min_count=1` so an all-null group stays `NaN`; or aggregate in DuckDB for both paths so the two agree; or, if the current behaviour is intended, document that `fillna=None` preserves only *absent coordinates* and not *stored nulls*, and say what a consumer should use instead to distinguish them.
Contributor guide
Research direction
Start in qpx/dataset.py at _abundance_sql and the in-memory pivot around lines 637-681, then compare the output_path pivot behavior. Run tests/integration/test_dataset.py around line 478 and reproduce the present-row NULL case. Done means fillna=None preserves a stored NULL and the in-memory and out-of-core paths agree, with regression coverage for that case.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- pandas, python, sql
- Domain
- data, databases
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 68/100