Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normalize pair coalescence counts/rates using non-missing span #3059

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

nspope
Copy link
Contributor

@nspope nspope commented Nov 20, 2024

Fixes #3053

Copy link

codecov bot commented Nov 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.86%. Comparing base (b31d4d1) to head (29f2c92).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3059   +/-   ##
=======================================
  Coverage   89.85%   89.86%           
=======================================
  Files          29       29           
  Lines       32137    32150   +13     
  Branches     5765     5768    +3     
=======================================
+ Hits        28877    28890   +13     
  Misses       1859     1859           
  Partials     1401     1401           
Flag Coverage Δ
c-tests 86.71% <100.00%> (+0.01%) ⬆️
lwt-tests 80.78% <ø> (ø)
python-c-tests 89.05% <ø> (ø)
python-tests 98.98% <ø> (ø)

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

Files with missing lines Coverage Δ
c/tskit/trees.c 90.70% <100.00%> (+0.02%) ⬆️
python/tskit/trees.py 98.80% <ø> (ø)
---- 🚨 Try these New Features:

Copy link
Member

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, great use of the num_edges count 👍

@jeromekelleher
Copy link
Member

We do need to update the changelog for this, though, as it's a significant bugfix

@nspope
Copy link
Contributor Author

nspope commented Nov 21, 2024

What's the best way to check for NAN in the C library? I see there's a tsk_is_unknown_time, but this returns false for tsk_is_unknown_time(NAN) on my system (Ubuntu 20.04 x86_64). Stack Overflow suggests that NAN != NAN is portable (guaranteed by IEEE 754) ... ?

Edit ah whoops I see there's an isnan macro in C99

Update changelog

Update changelog

Docstring fix, more tests
@nspope nspope force-pushed the fix-coalrate-missing-span branch from e6ec5f7 to 29f2c92 Compare November 21, 2024 02:51
@nspope
Copy link
Contributor Author

nspope commented Nov 21, 2024

The changelog is updated, and I added some tests checking the behavior of the statistics on empty windows. In particular, pair_coalescence_counts will return zeros (rather than NaNs) if span_normalise=True and a window has no nonmissing span; the other two functions will return NaNs.

Copy link
Member

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Great to see these awkward corner cases getting hit so soon, this stuff is definitely getting used!

@jeromekelleher jeromekelleher added the AUTOMERGE-REQUESTED Ask Mergify to merge this PR label Nov 21, 2024
@mergify mergify bot merged commit 8cd494a into tskit-dev:main Nov 21, 2024
21 checks passed
@mergify mergify bot removed the AUTOMERGE-REQUESTED Ask Mergify to merge this PR label Nov 21, 2024
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.

pair_coalescence_rate doesn't account for empty regions (e.g. flanks)
2 participants