make sequence_length override ts.sequence_length in argrecoder init

[petrelharp]

This should close #45 .

[codecov[bot]]

Codecov Report

Merging #46 into master will increase coverage by 0.27%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #46      +/-   ##
==========================================
+ Coverage   75.37%   75.65%   +0.27%     
==========================================
  Files           4        4              
  Lines         264      267       +3     
==========================================
+ Hits          199      202       +3     
  Misses         65       65

Impacted Files	Coverage Δ
ftprime/argrecorder.py	73.59% <100%> (+0.45%)	:arrow_up:

---

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 726dc9c...f4b897c. Read the comment docs.

[ashander]

One issue: If we actually pass a seq len that is < seq len defined by the edges in the initial ts , this will cause an error (see the commit I just pushed)

[ashander]

pasting the error from circle here for convenience

self = <tests.test_arg_recorder.BasicTestCase testMethod=test_simplify2>

    def test_simplify2(self):
        # test that we get the same tree sequence by doing tree_sequence
        # and simplify -> tree_sequence
        records = ftprime.ARGrecorder(ts=self.init_ts, node_ids=self.init_map,
                                      sequence_length=0.5)
        records.add_individual(4, 2.0, population=2)
        records.add_individual(5, 2.0, population=2)
        records.add_record(0.0, 0.5, 0, (4, 5))
        records.add_record(0.5, 1.0, 0, (4,))
        print(records)
>       tsa = records.tree_sequence([4, 5])

tests/test_arg_recorder.py:109: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
ftprime/argrecorder.py:352: in tree_sequence
    sequence_length=self.sequence_length)
src/msprime/msprime/trees.py:810: in load_tables
    return TreeSequence.load_tables(*args, **kwargs)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

cls = <class 'msprime.trees.TreeSequence'>
kwargs = {'edges': <msprime.tables.EdgeTable object at 0x7f08090c1988>, 'mutations': <msprime.tables.MutationTable object at 0x7f08090c1348>, 'nodes': <msprime.tables.NodeTable object at 0x7f08090c1048>, 'sequence_length': 0.5, ...}
ts = <_msprime.TreeSequence object at 0x7f0809feed20>

    @classmethod
    def load_tables(cls, **kwargs):
        ts = _msprime.TreeSequence()
>       ts.load_tables(**kwargs)
E       _msprime.LibraryError: Right coordinate > sequence length.

src/msprime/msprime/trees.py:1051: LibraryError

[petrelharp]

An informative error, as expected. But i've added a check for this and a possibly more informative error. Ready to merge now?

[ashander]

Fair point on the valid use cases. Because the LibraryError is informative maybe we should actually revert everything back in https://github.com/ashander/ftprime/blob/f4b897c5d40cfaf206c1f0cdaa82f35f7a80218d/ftprime/argrecorder.py to your original commit in this PR (we could change the AssertRaises to check for LibraryError or just delete it). Otherwise we run risk of our input checking blocking the valid uses that you metion

On Fri, Oct 20, 2017 at 9:43 PM Peter Ralph notifications@github.com wrote:

An informative error, as expected. But i've added a check for this and a possibly more informative error. Ready to merge now?

— You are receiving this because you commented.

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

-- -Jaime

[petrelharp]

I think it's good as it currently stands. The use cases I mention are not actually likely; I was just explaining my previous thinking.

On Fri, Oct 20, 2017 at 10:25 PM, ashander notifications@github.com wrote:

Fair point on the valid use cases. Because the LibraryError is informative maybe we should actually revert everything back in https://github.com/ashander/ftprime/blob/f4b897c5d40cfaf206c1f0cdaa82f3 5f7a80218d/ftprime/argrecorder.py to your original commit in this PR (we could change the AssertRaises to check for LibraryError or just delete it). Otherwise we run risk of our input checking blocking the valid uses that you metion

On Fri, Oct 20, 2017 at 9:43 PM Peter Ralph notifications@github.com wrote:

An informative error, as expected. But i've added a check for this and a possibly more informative error. Ready to merge now?

— You are receiving this because you commented.

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

-- -Jaime

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ashander/ftprime/pull/46#issuecomment-338365361, or mute the thread https://github.com/notifications/unsubscribe-auth/AA_26dEk191wlv0GDE0hvd8Vz2WlXFiIks5suYAtgaJpZM4QBIqS .

[ashander]

👍

On Sat, Oct 21, 2017 at 10:01 AM Peter Ralph notifications@github.com wrote:

I think it's good as it currently stands. The use cases I mention are not actually likely; I was just explaining my previous thinking.

On Fri, Oct 20, 2017 at 10:25 PM, ashander notifications@github.com wrote:

Fair point on the valid use cases. Because the LibraryError is informative maybe we should actually revert everything back in https://github.com/ashander/ftprime/blob/f4b897c5d40cfaf206c1f0cdaa82f3 5f7a80218d/ftprime/argrecorder.py to your original commit in this PR (we could change the AssertRaises to check for LibraryError or just delete it). Otherwise we run risk of our input checking blocking the valid uses that you metion

On Fri, Oct 20, 2017 at 9:43 PM Peter Ralph notifications@github.com wrote:

An informative error, as expected. But i've added a check for this and a possibly more informative error. Ready to merge now?

— You are receiving this because you commented.

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

-- -Jaime

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ashander/ftprime/pull/46#issuecomment-338365361, or mute the thread < https://github.com/notifications/unsubscribe-auth/AA_26dEk191wlv0GDE0hvd8Vz2WlXFiIks5suYAtgaJpZM4QBIqS

.

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

-- -Jaime