Conversation
leehart
left a comment
There was a problem hiding this comment.
Thanks @jonbrenas .
I'm not sure about the assert. I suppose the idea is that it will fail if either the number of samples or the number of SNPs is 3 or fewer. I expect there would be a ValueError raised in randint if n_snps_available was fewer than 4, so I expect n_snps will be at least 4 at the assert and so it might boil down to n_samples. Perhaps we might want a way to either ensure enough samples and SNPs at this point, or otherwise raise a clear error when that hasn't happened. But perhaps that's a different problem for a different day!
|
By the way, I don't quite understand the commit that "Drops a few unused columns", which doesn't come through as a change, presumably due to having an identical change being already merged in from master? Was that commit related to the same issue, or an artefact of something else perhaps? |
|
Thanks @leehart . The The commits have been part of all my PRs for a while. I am not sure why. I hope they disappear once one of these PRs is merged into master. |
|
Thanks @jonbrenas
|
|
It actually fails on exactly the test that I modified because it can sometimes randomly fail on unrelated PRs (but this PR has to pass the previous tests for it to be approved, obviously, which it now does). |
|
Haha, I see, of course. Thanks @jonbrenas . OK if I delete the branch from GitHub now it's been merged in? |
|
Done! |
Addresses #618 .