ashander / ftprime

Forward-time simulation of the msprime data structure (for development)
2 stars 1 forks source link

removed samples from the slots 0...n-1 as in msprime:generalise-samples #26

Closed petrelharp closed 7 years ago

petrelharp commented 7 years ago

Should be straightforward.

I've fixed up the tests some, too - they don't always make genealogical sense. And added a check that an individual doesn't get inserted more than once.

ashander commented 7 years ago

I updated (temporarily) the environment.yml for CI tests to generalise-samples branch of msprime

ashander commented 7 years ago

Still failing, but now with errors in .simplify(), which you referred to in https://github.com/jeromekelleher/msprime/pull/194

example:

>       ts.simplify(samples=list(range(nsamples)))

tests/test_recomb_collector.py:131: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <msprime.trees.TreeSequence object at 0x7fa19dee17f0>, samples = [0, 1]
filter_root_mutations = True

    def simplify(self, samples=None, filter_root_mutations=True):
        if samples is None:
            samples = self.get_samples()
        ll_ts = _msprime.TreeSequence()
>       self._ll_tree_sequence.simplify(ll_ts, samples, filter_root_mutations)
E       ValueError: Specified node is not a sample
codecov[bot] commented 7 years ago

Codecov Report

Merging #26 into master will decrease coverage by 0.6%. The diff coverage is 94.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #26      +/-   ##
==========================================
- Coverage   99.36%   98.76%   -0.61%     
==========================================
  Files           3        3              
  Lines         158      162       +4     
==========================================
+ Hits          157      160       +3     
- Misses          1        2       +1
Flag Coverage Δ
#unit 65.43% <57.89%> (+1.5%) :arrow_up:
Impacted Files Coverage Δ
ftprime/recomb_collector.py 100% <100%> (ø) :arrow_up:
ftprime/argrecorder.py 98.05% <90%> (-0.92%) :arrow_down:

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 17f2ab5...61dfc9e. Read the comment docs.

petrelharp commented 7 years ago

Since this change removes the main reason we needed to do bending-around-backwards yoga, I decided to remove the other reason for it - backwards time - so now argrecorder works entirely in forwards time. It keeps track of the maximum time seen so far, then when outputting a NodeTable (eg to msprime) it subtracts all the forwards-times from this maximum time to get time-ago.

This means we no longer need to tell RecombCollector how many generations there are going to be - so I have removed that argument from init. I believe there are no other UI changes.

petrelharp commented 7 years ago

Would like to merge?

ashander commented 7 years ago

looks good to merge On Thu, Apr 27, 2017 at 10:19 PM Peter Ralph notifications@github.com wrote:

Would like to merge?

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/ashander/ftprime/pull/26#issuecomment-297909316, or mute the thread https://github.com/notifications/unsubscribe-auth/AAfLOOSdb3x99A6nO8qlT54hFhzFLPi6ks5r0XbIgaJpZM4NJ1py .

-- -Jaime

petrelharp commented 7 years ago

whoops, forgot your comments. done those.

ashander commented 7 years ago

oops I misunderstood that msprime PR, because thought it showed internal nodes as samples On Thu, Apr 27, 2017 at 10:24 PM Peter Ralph notifications@github.com wrote:

@petrelharp commented on this pull request.

In ftprime/argrecorder.py https://github.com/ashander/ftprime/pull/26#discussion_r113854259:

@@ -122,21 +125,25 @@ def tree_sequence(self, sites=None, mutations=None):

 def add_samples(self, samples, length, populations=None):
     '''
  • Add phony records that stand in for sampling the IDs in samples,
  • Set the sample flag on the samples,
  • and optionally set their populations.
  • Previously: Add phony records that stand in for sampling the IDs in samples, whose populations are given in populations (default: NULL), on a chromosome of total length length.

Except that currently can't label an internal node as a sample.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ashander/ftprime/pull/26#discussion_r113854259, or mute the thread https://github.com/notifications/unsubscribe-auth/AAfLOIE8MxCiPxGfbIs2ElQtao6Ven3uks5r0XgRgaJpZM4NJ1py .

-- -Jaime