Skip to content

Stabilize trait correlation under large common weight offsets - #3494

Closed
ArtificialZeng wants to merge 1 commit into
tskit-dev:mainfrom
ArtificialZeng:codex/trait-correlation-centering
Closed

ArtificialZeng wants to merge 1 commit into
tskit-dev:mainfrom
ArtificialZeng:codex/trait-correlation-centering

Conversation

@ArtificialZeng

@ArtificialZeng ArtificialZeng commented Oct 27, 2026 •

Copy link
Copy Markdown

trait_correlation can change when a common offset is added to a finite, positive-variance trait. For four samples with genotype (0,0,1,1) and weights (0,1,2,3), the exact squared correlation is 0.8. At pinned main f154d119, adding 2**26 returns approximately 1.0; adding 2**27 returns NaN. All input integers remain exactly representable.

The shared C implementation computes variance as sum(w*w)-n*mean(w)**2, losing the small centered variance. The patch first subtracts the first sample of each trait, computes the residual mean, and accumulates squared centered residuals before normalization. This supplies the same sample variance in exact arithmetic and stabilizes the common-offset case in site, branch and node modes. Python/C changelogs are included.

Validation on native current-source baseline and candidate builds:

  • 12 new regression cases: baseline 12 failures; candidate 12 passes. These use explicit Pearson values for two traits and allele/node/branch sets.
  • 52 existing trait-correlation tests pass.
  • A separate AI context constructed an independent Fraction oracle from explicit graph descendants and allele partitions: baseline 312/396 failures, candidate 396/396 passes, including n=2/4/6, nonconsecutive sample nodes, two trait columns, positive/negative offsets, windows and span normalization.
  • Ruff0.15.1, clang-format21.1.8 and diff checks pass.

The full optional-dependency suite was not run locally. This is a classical numerical-stability correction; extreme dynamic-range overflow/underflow is outside the demonstrated fix, and no effect on published biological analyses is claimed. Development and independent checking used substantial AI assistance.

Self-contained counterpart: ArtificialZeng/AI-Biology-Lab#38 (current documentation/status head a478a224128f791b38f933159426a90c2b06ac6f, 9/9 checks). Dedicated run 40515256247 performed four fresh paired native replays/eight builds across Linux/macOS and normal/optimized parents. The scientific patch and reproduction code are unchanged from the earlier validated packet; all four patched source files match this upstream head.

Upstream validation at current head 369ff4fce5eda3ab4dd25366c8812af9df3a8149: all 17 PR status checks pass, including the 13-job Tests workflow (run 40505067326, attempt 2), lint and documentation. This hosted validation supplements the targeted local tests above; the full optional-dependency suite was not rerun locally.

@jeromekelleher

Copy link
Copy Markdown
Member

This looks good to me - @petrelharp, I think this is your code originally, want to take a look?

@codecov

codecov Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.68%. Comparing base (f154d11) to head (369ff4f).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3494   +/-   ##
=======================================
  Coverage   91.68%   91.68%           
=======================================
  Files          38       38           
  Lines       32242    32245    +3     
  Branches     5155     5158    +3     
=======================================
+ Hits        29560    29563    +3     
  Misses       2346     2346           
  Partials      336      336           
Flag Coverage Δ
C 82.26% <100.00%> (+<0.01%) ⬆️
c-python 77.53% <100.00%> (+<0.01%) ⬆️
python-tests 96.44% <ø> (ø)
python-tests-no-jit 33.29% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
Python API 98.73% <ø> (ø)
Python C interface 91.25% <ø> (ø)
C library 91.22% <100.00%> (+<0.01%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@petrelharp

Copy link
Copy Markdown
Contributor

Briefly: I would like to not merge this PR. It's true that this code could be better about avoiding overflow, but this only fixes one of the issues with it. (Note also that it adds up all the values then divides by the number, which is probably a bigger problem.) And this code looks right. But I don't want to spend my time merging in drive-by autonomously-generated minor fixes unless it's blindingly obvious it's the right thing to do.

Also note that this is being careful about the weights (the traits; W in ts.trait_correlation), which are user-inputted. If the user is inputting traits that suffer from numerical overflow (so numbers of order 2^26 = 10^7-ish according to this issue) then a simple fix is to use different units.

I'll open an issue to fix up the overflow in this part of the code, though.

@jeromekelleher - reopen if you disagree?

@petrelharp petrelharp closed this Oct 27, 2026
@petrelharp

Copy link
Copy Markdown
Contributor

See #3495

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants