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

Numerical stability of beta model #2257

Merged
merged 1 commit into from Feb 8, 2024

Conversation

JereKoskela
Copy link
Member

Fixing a bias in Beta-coalescent TMRCAs due to numerical issues with acceptance probabilities.

Closes #2256

Copy link

codecov bot commented Feb 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (804e036) 98.70% compared to head (dcf123a) 91.52%.

❗ Current head dcf123a differs from pull request most recent head fdc169c. Consider uploading reports for the commit fdc169c to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2257      +/-   ##
==========================================
- Coverage   98.70%   91.52%   -7.19%     
==========================================
  Files          11       20       +9     
  Lines        4022    11337    +7315     
  Branches      907     2304    +1397     
==========================================
+ Hits         3970    10376    +6406     
- Misses         28      523     +495     
- Partials       24      438     +414     
Flag Coverage Δ
C 91.52% <100.00%> (?)
python 98.70% <ø> (ø)

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

Files Coverage Δ
lib/msprime.c 86.12% <100.00%> (ø)

... and 8 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 804e036...fdc169c. Read the comment docs.

@jeromekelleher
Copy link
Member

LGTM. Can you update the CHANGELOG please to note this bugfix?

Shall we push out a quick bugfix release?

@JereKoskela
Copy link
Member Author

I'm guessing that the CHANGELOG should have an entry along these lines:

Bug fixes:

Change tolerance of polynomial approximation in Beta-coalescent acceptance probabilities ({issue}2256, {pr}2257, {user}JereKoskela)

But what should the header be? All existing entries are written under version numbers of the corresponding release, and this one doesn't have one (at least yet). I'm happy for you to enter this in too if that's quicker than explaining the process to me @jeromekelleher.

As for whether to release a bugfix, I have no idea what that entails so happy to leave the decision to you. I doubt this is having a big impact and waiting a little while is unlikely to cause havoc if it would be simpler to wait until a more substantial release is ready.

@GertjanBisschop
Copy link
Member

It is as simple as providing a provisional header @JereKoskela.
[1.3.1] - 2024-XX-XX

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.

Perfect, thanks @JereKoskela

@jeromekelleher
Copy link
Member

OK, maybe just squash those commits now @JereKoskela and we'll merge.

I think we'll just pop out a quick release. This is the only change and it's easy to do. Are you happy to shepherd the release process @GertjanBisschop ?

@JereKoskela
Copy link
Member Author

Squash done. Over to you @jeromekelleher and @GertjanBisschop

@mergify mergify bot merged commit 7ee1619 into tskit-dev:main Feb 8, 2024
12 checks passed
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.

Discrepancy between expected and observed coalescence time in beta coalescent for large values of alpha
3 participants