Fix ks_statistic_gene_cell metrics being dropped from the results - #17
Merged
Conversation
The cutoff of 15 dropped every uns entry longer than 15 elements. ks_statistic_gene_cell emits 28 metric_ids, so extract_uns_metadata threw them away and the report could not attribute any of its values -- all 28 metrics showed up as 100% missing in run_2026-07-11_18-00-02.
`ks::kde.test()` returns a list with fields `Tstat`, `zstat`, `pvalue`, ... so `result$tstat` was NULL and `c()` dropped it without complaining. On the success path only 14 of the 28 values were emitted, on the fallback path all 28 -- so metric_values had a length somewhere between 14 and 28 while metric_ids always had 28. Until now the uns_length_cutoff hid this by dropping both vectors. With the cutoff raised the two would be zipped together positionally, which mislabels scores rather than losing them, so this has to land in the same PR.
Two things were being mixed into one metric. `zstat` and `Tstat` are unbounded -- across run_2026-07-11_18-00-02 zstat spans roughly -6 to +113 -- whereas the empirical KS statistic lives in 0..1, so a fallback value of ~0.1 outranked nearly every genuine result. `maximize` is false, so lower is better and failing was the best thing a simulator could do. The same goes for the "worst-case" penalty of 1, which is near-best on this scale. There is no finite value that reads as "worst" on an unbounded scale, so report NA instead. That way a crash shows up as missing in the QC rather than as a suspiciously good score. This reverts the fallback introduced in 3301ded and drops `empirical_ks_statistic()` and `sliced_ks_statistic()` with it.
This is the other half of the NA problem 518bc67 set out to fix, but solved on the kde scale rather than by swapping in a different statistic. * Restore the deterministic jitter retry, which is what `ks_statistic_sc_features` still does. kde.test trips over ties, which count data has plenty of; nudging the inputs through 1e-8 .. 1e-4 gets it through. A tied input that used to error now returns zstat 4.07. * Guard against inputs with no variance. Both sides get the same deterministic noise, so a constant input stays identical after jittering and kde.test then reports it as far more alike than chance -- a top score for degenerate data. Those now report NA.
rcannood
force-pushed
the
fix/ks-statistic-metric-ids
branch
from
July 28, 2026 11:26
fa14106 to
5232640
Compare
8 tasks
Contributor
|
Thanks @rcannood ! |
This was referenced Jul 28, 2026
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
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.
Describe your changes
Rebuilt on top of 3301ded.
ks::kde.test()stays the metric, as @ycao6928 intended -- this PR makes the plumbing around it correct.In the last full run (
27Klprxf2344VO,run_2026-07-11_18-00-02) everyks_statistic_gene_cellmetric came out as 100% missing -- 0 results where 110 were expected, for all 28 of them.The cutoff.
extract_uns_metadataonly keeps a uns entry with at mostuns_length_cutoffelements, andrun_benchmarkpassed 15. The component emits 28metric_ids, so the ids were thrown away and the report had no way to attribute the values. Raised to 50.$tstatis not a field.ks::kde.test()returnsTstat,zstat,pvalue, ..., soresult$tstatwas NULL andc()dropped it silently:On the success path only 14 of 28 values came out; on the fallback path all 28 -- so
metric_valueshad a length anywhere between 14 and 28 against 28metric_ids. The cutoff was hiding this, and raising it on its own would have zipped the two together positionally and mislabelled scores rather than losing them. That's why both changes are in one PR.Failure was the best possible outcome.
zstatandTstatare unbounded -- zstat spans about -6 to +113 across the last run -- while the empirical KS fallback lives in 0..1 and the penalty was a literal1. Withmaximize: falseboth rank above nearly every genuine result, so a component that crashed scored better than one that worked. There's no finite value that reads as "worst" on an unbounded scale, so failures now report NA and show up as missing in the QC instead.Which brings back the NAs you were fixing, so I fixed those on the kde scale instead. Restored the deterministic jitter retry that
ks_statistic_sc_featuresstill uses -- kde.test trips over ties, which count data has plenty of, and nudging the inputs through1e-8 .. 1e-4gets it through. A tied input that used to error now returnszstat4.07. Plus a guard for inputs with no variance: both sides get the same deterministic noise, so a constant input stays identical after jittering and kde.test then calls it far more alike than chance -- a top score for degenerate data. Those report NA.Behaviour after this PR, checked against the component's own image:
and on realistic data, an identical (positive-control) pair gives
zstat = -0.8against+1.2for a mildly wrong simulator, consistent with the-1.246we saw for the positive control in the real run.Config left alone: 28 metrics,
min: -Inf/max: +Inf/maximize: falseall still match what the component emits.One thing worth being aware of rather than fixing here:
zstatis a standardised test statistic, so at a fixed effect size it grows with n -- mean 2.70 at n=200 up to 25.76 at n=5000 over 20 seeds. Across 10 datasets of different sizes it partly ranks by dataset size. Happy to open a separate issue if you want to think about that.Checklist before requesting a review
I have performed a self-review of my code
Check the correct box. Does this PR contain:
Proposed changes are described in the CHANGELOG.md
CI Tests succeed and look good!