Add ts.coiterate method #1022
Add ts.coiterate method #1022
Conversation
|
@benjeffery - this is failing because sphinx can't find the |
There was a problem hiding this comment.
A few nits and suggestions. The kwargs functionality is not being tested yet. There are also some failing tests that refer to the old tsutil version of this. The doc build is failing as Interval is not documented, i.e. python-api.rst doesn't contain .. autoclass:: Interval. The class itself is also missing docstrings.
All great suggestions, thanks @benjeffery . I didn't realise that documentation was required to link (although I guess that's obvious, really), Should I document Interval, even though it's trivial? |
If we're returning iterators using it then I guess it goes in the "Simple Container Classes" section. |
|
📖 Docs for this PR can be previewed here |
Codecov Report
@@ Coverage Diff @@
## main #1022 +/- ##
=======================================
Coverage 93.68% 93.69%
=======================================
Files 26 26
Lines 20829 20846 +17
Branches 855 859 +4
=======================================
+ Hits 19514 19531 +17
Misses 1277 1277
Partials 38 38
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
All done. Thanks @benjeffery |
There was a problem hiding this comment.
Very nice, thanks @hyanwong. A couple of small tweaks and we're good to merge.
| ts1 = msprime.simulate(4, random_seed=1, length=2) | ||
| ts2 = msprime.simulate(4, random_seed=1) | ||
| with pytest.raises(ValueError, match="equal sequence length"): | ||
| next(ts1.coiterate(ts2)) |
There was a problem hiding this comment.
We shouldn't need the next here I think
There was a problem hiding this comment.
I think we do. If I remove it, we don't get the error raised, because we don't actually start the generator going?
There was a problem hiding this comment.
Generator functions are only executed when after the first next so yeah, we do need it.
| @@ -3850,6 +3863,43 @@ def trees( | |||
| ) | |||
| return TreeIterator(tree) | |||
|
|
|||
| def coiterate(self, other, **kwargs): | |||
There was a problem hiding this comment.
Actually, might make more sense to put this next to the trees iterator in the file? It's nice to keep methods somewhat grouped by topic.
There was a problem hiding this comment.
Erm. That's where it is, isn't it?
There was a problem hiding this comment.
Yep, so it is. GitHub's way of showing context is a bit weird sometimes
hyanwong
force-pushed
the
coiterate
branch
2 times, most recently
from
39eadef to
79be302
Compare
AdminBot-tskit
force-pushed
the
coiterate
branch
from
79be302 to
06b8be6
Compare
Fixes #1021