tskit-dev / msprime

Simulate genealogical trees and genomic sequence data using population genetic models
GNU General Public License v3.0
173 stars 85 forks source link

Numerical stability of beta model #2257

Closed JereKoskela closed 7 months ago

JereKoskela commented 7 months ago

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

Closes #2256

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

:exclamation: 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](https://app.codecov.io/gh/tskit-dev/msprime/pull/2257/graphs/tree.svg?width=650&height=150&src=pr&token=1uZQV1KMkU&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev)](https://app.codecov.io/gh/tskit-dev/msprime/pull/2257?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev) ```diff @@ 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](https://app.codecov.io/gh/tskit-dev/msprime/pull/2257/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev) | Coverage Δ | | |---|---|---| | [C](https://app.codecov.io/gh/tskit-dev/msprime/pull/2257/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev) | `91.52% <100.00%> (?)` | | | [python](https://app.codecov.io/gh/tskit-dev/msprime/pull/2257/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev) | `98.70% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/tskit-dev/msprime/pull/2257?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev) | Coverage Δ | | |---|---|---| | [lib/msprime.c](https://app.codecov.io/gh/tskit-dev/msprime/pull/2257?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev#diff-bGliL21zcHJpbWUuYw==) | `86.12% <100.00%> (ø)` | | ... and [8 files with indirect coverage changes](https://app.codecov.io/gh/tskit-dev/msprime/pull/2257/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev) ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/tskit-dev/msprime/pull/2257?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://app.codecov.io/gh/tskit-dev/msprime/pull/2257?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev). Last update [804e036...fdc169c](https://app.codecov.io/gh/tskit-dev/msprime/pull/2257?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev).
jeromekelleher commented 7 months ago

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

Shall we push out a quick bugfix release?

JereKoskela commented 7 months ago

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 commented 7 months ago

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

jeromekelleher commented 7 months ago

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 commented 7 months ago

Squash done. Over to you @jeromekelleher and @GertjanBisschop