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

Add Deme.size_at() to compute the deme size at a given time. #314

Merged
merged 1 commit into from
Jun 4, 2021

Conversation

grahamgower
Copy link
Member

Closes #312.

@codecov
Copy link

codecov bot commented Jun 3, 2021

Codecov Report

Merging #314 (7c9950b) into main (083dcea) will increase coverage by 0.08%.
The diff coverage is 100.00%.

❗ Current head 7c9950b differs from pull request most recent head a9db443. Consider uploading reports for the commit a9db443 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #314      +/-   ##
==========================================
+ Coverage   98.15%   98.24%   +0.08%     
==========================================
  Files           8        8              
  Lines        1573     1595      +22     
==========================================
+ Hits         1544     1567      +23     
+ Misses         29       28       -1     
Impacted Files Coverage Δ
demes/demes.py 100.00% <100.00%> (ø)
demes/hypothesis_strategies.py 98.68% <100.00%> (+0.73%) ⬆️

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 083dcea...a9db443. Read the comment docs.

Copy link
Member

@apragsdale apragsdale left a comment

Choose a reason for hiding this comment

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

Looking good - just a small suggestion for the error message.

demes/demes.py Outdated Show resolved Hide resolved
@grahamgower grahamgower merged commit b67a064 into popsim-consortium:main Jun 4, 2021
:rtype: float
"""
for epoch in self.epochs:
if epoch.start_time >= time >= epoch.end_time:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's the sleep deprivation, but I can't get my head around this condition - shouldn't it be epoch.start_time <= time < epoch.end_time? As in, we break if the time is within this epoch's interval?

Copy link
Member Author

@grahamgower grahamgower Jun 4, 2021

Choose a reason for hiding this comment

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

Two things: (1) start_time > end_time, so we must have start_time > time >= end_time. And (2) we also want to accept time=math.inf, so we check start_time >= time >= end_time. Given this is in a loop, and the epochs are time-sorted, the start_time == time condition is only hit for the first epoch, and only if the time is infinity.

Copy link
Member

Choose a reason for hiding this comment

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

Ack, I'm thinking backwards in time as usual, thanks, my bad!

@grahamgower grahamgower deleted the size-at branch June 4, 2021 11:09
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.

Compute size of deme at time t
3 participants