Skip to content

PBS #368

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

Merged
merged 3 commits into from
Nov 4, 2020
Merged

PBS #368

merged 3 commits into from
Nov 4, 2020

Conversation

tomwhite
Copy link
Collaborator

@tomwhite tomwhite commented Nov 2, 2020

Fixes #230

Copy link
Collaborator

@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.

I like it, but minor query on numerics

variables.validate(ds, {stat_Fst: variables.stat_Fst_spec})

fst = ds[variables.stat_Fst]
fst = fst.clip(min=0, max=0.99999)
Copy link
Collaborator

Choose a reason for hiding this comment

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

0.99999 feels a bit arbitrary and I worry this will lead to numerical artefacts for values of Fst near 1. Something like

>>> 1 - np.finfo(float).epsneg
0.9999999999999999
>>> np.log(1 - np.finfo(float).epsneg)
-1.1102230246251565e-16

seems a bit more defensible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This looks very reasonable, and I've updated this PR in the way you suggest. cc-ing @alimanfoo since the value 0.99999 came from scikit-allel's implementation: https://github.com/cggh/scikit-allel/blob/master/allel/stats/selection.py#L1323

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've marked this as automerge as I doubt there was any deep reason for the 5-digit 0.9999.

@codecov-io
Copy link

codecov-io commented Nov 3, 2020

Codecov Report

Merging #368 into master will decrease coverage by 0.34%.
The diff coverage is 67.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #368      +/-   ##
==========================================
- Coverage   95.53%   95.18%   -0.35%     
==========================================
  Files          31       31              
  Lines        2239     2266      +27     
==========================================
+ Hits         2139     2157      +18     
- Misses        100      109       +9     
Impacted Files Coverage Δ
sgkit/stats/popgen.py 67.33% <65.38%> (-0.41%) ⬇️
sgkit/__init__.py 100.00% <100.00%> (ø)
sgkit/variables.py 96.46% <100.00%> (+0.03%) ⬆️

Continue to review full report at Codecov.

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

@jeromekelleher jeromekelleher added the auto-merge Auto merge label for mergify test flight label Nov 4, 2020
@mergify mergify bot merged commit 8cf1a80 into sgkit-dev:master Nov 4, 2020
@tomwhite tomwhite mentioned this pull request Nov 5, 2020
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge Auto merge label for mergify test flight
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PBS selection statistic
3 participants