Repository navigation
Conversation
…ete site ac.diversity.max_daf used -1.0 as a sentinel to make an incomplete (site, allele) column lose the max comparison under missing_data='exclude'. That only works if some column is eligible; when every site has a missing call, every column is -1.0, and the max returns the sentinel itself -- outside the documented [0, 1] range (#340). Fixed by checking directly whether any site is complete first: NaN when none is (nothing to measure), otherwise fill every ineligible column with 0.0 (safe now that a complete site is known to exist, matching the existing "no derived allele present" convention) and take the max as before. This also fixes the subtler case of a complete site that carries no derived allele -- previously also -1.0, now correctly 0.0, not NaN, since there is data to measure, just no derived allele in it. Tests: the issue's own reproducer (no complete site -> NaN), and a complete-but-monomorphic-site case pinned to 0.0 to cover that distinction explicitly. Also fixes the pipeline_map.toml undefined text and adds a changelog note.
The scalar max_daf kept a site with as little as one call (n_valid > 0), while both windowed engines already required a site's own pair (n_valid >= 2) -- the package convention from that pi/theta_w/ heterozygosity_expected also follow, since a frequency estimate from one call is not useful. A single stray derived-allele call could set the whole-region scalar value to that call's own frequency (up to 1.0) while the windowed value, on the same data, correctly ignored it. Bumped the scalar's include-mode gate to n_valid >= 2 to match. Docstring now says so explicitly. Tests: the issue's own reproducer, pinned to the windowed value (1/3, not 1.0), plus a direct comparison against both the scatter and fused engines on the same data. Checked the existing missing-data parity fixture in test_implementation_parity.py always keeps at least two valid calls per site, so it doesn't exercise this gate either way and needed no change. Also updates the now-stale pipeline_map.toml note (it previously documented the scalar/windowed mismatch as a known fact) and the changelog.
The scalar max_daf(missing_data='exclude') now gives NaN when no site is complete (previous commit). The windowed engines had the same gap one level up: _windowed_thetas_scatter, windowed_statistics_fused, and windowed_statistics_fused_chunked all filter the whole matrix to complete sites once, upfront, before windowing even starts. By the time a window's variant count is checked, a window that originally held variants but lost them all to exclusion is indistinguishable from a window that never had a variant to begin with -- both just have zero post-filter variants, and both engines left max_daf at its zero-initialized default either way. The first case is undefined (nothing to measure); the second is the established "0.0 for an empty window" convention every additive statistic already uses, which does NOT apply to a max. Fixed by capturing each window's variant count from the ORIGINAL (pre-filter) positions before exclusion runs, alongside the existing post-filter count: a window with pre-filter variants but zero post-filter ones gets NaN, mirroring the existing zero_span post-hoc override pattern in the scatter engines (compute a per-window flag once, apply it via np.where at the point the result is built) rather than threading a special case through the max-accumulation logic itself. Needed in all three engines, including the chunked one, since chunking only changes how the kernel is invoked, not how the window-to-variant-index ranges are set up before it. Tests: a two-window matrix (one window has two sites each with a missing call -- wiped out by exclude; the other has no variant at all) run through all three engines plus the scalar function on the equivalent subset, confirmed to fail without the fix. Also updates the pipeline_map.toml undefined/notes text and the changelog.
nspope
force-pushed
the
fix/max-daf-exclude-nan
branch
from
November 6, 2026 05:33
7e48de2 to
a96eb1d
Compare
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.
Closes #376 and #340
This fixes three related correctness bugs in max_daf: the scalar function returns -1.0 (outside its documented [0, 1] range) instead of NaN when missing_data='exclude' leaves no complete site; the scalar function's include-mode threshold let a single-call site set the result, disagreeing with the windowed engines, which already required a site's own pair (n_valid >= 2) — now both require it; and all three windowed engines (windowed_analysis, windowed_statistics_fused, windowed_statistics_fused_chunked) gave 0.0 instead of NaN when exclude emptied a window that had data, conflating that case with a window that never had any variant. Each fix includes regression tests and updated changelog/pipeline-map documentation.