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 Canis familiaris to species catalog. #375

Merged
merged 1 commit into from
Jan 10, 2020

Conversation

grahamgower
Copy link
Member

TODO: species specific tests and rtd stuff. Anything else missing?

@codecov
Copy link

codecov bot commented Jan 8, 2020

Codecov Report

Merging #375 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #375      +/-   ##
==========================================
+ Coverage   99.48%   99.49%   +<.01%     
==========================================
  Files          14       15       +1     
  Lines        1164     1179      +15     
  Branches      120      121       +1     
==========================================
+ Hits         1158     1173      +15     
  Misses          2        2              
  Partials        4        4
Impacted Files Coverage Δ
stdpopsim/catalog/canis_familiaris.py 100% <100%> (ø)

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 fc9cc66...ca39738. Read the comment docs.

@andrewkern
Copy link
Member

looks good. just missing the RTD stuff and tests, yeah

grahamgower added a commit to grahamgower/stdpopsim that referenced this pull request Jan 9, 2020
This needs popsim-consortium#375 to unbreak the RTD build.
@andrewkern
Copy link
Member

was the RTD thing with the citations to blame here for the ci error?

@grahamgower
Copy link
Member Author

Yep. And as far as I can tell, the unit tests that are missing from this PR would only be like those I'm complaining about in #376.

@andrewkern
Copy link
Member

just looked at that issue. for now lets stick to including too many unit tests i think and if you agree we can get this one merged

@grahamgower
Copy link
Member Author

Ok, I added the unit test file. In the spirit of providing solutions, and not just criticism, I put in an end-to-end test (i.e. test that this runs: stdpopsim CanFam -c chr38 -l 0.001 --seed 1234 -q 10).

@jeromekelleher
Copy link
Member

This is excellent, thanks @grahamgower! It would be nice to add tests for pop size as I discussed in #376, but otherwise LGTM.

@jeromekelleher
Copy link
Member

I'm happy to merge here. I'll let @andrewkern push the button in case there's anything he spots.

@andrewkern
Copy link
Member

looks really good. i like the end-to-end test especially @grahamgower. this might be a good standard to adopt for each species.

@andrewkern andrewkern merged commit 88a1cf6 into popsim-consortium:master Jan 10, 2020
@grahamgower grahamgower deleted the dog branch January 16, 2020 15:32
@petrelharp petrelharp mentioned this pull request May 11, 2022
14 tasks
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.

3 participants