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

Make TableCollections iterable #244

Merged
merged 1 commit into from
Jun 25, 2019

Conversation

hyanwong
Copy link
Member

No test cases yet, I'll write them once the idea is approved

@hyanwong hyanwong force-pushed the tables-iterator branch 2 times, most recently from 1d59c57 to 439fe98 Compare June 20, 2019 19:52
@hyanwong
Copy link
Member Author

So now instead of @jeromekelleher 's ts identify checking code it's now possible to do

assert all(t1==t2 for t1, t2 in zip(ts1.tables, ts2.tables) if t1[0] != 'provenances')

Which I think is a little cleaner. Or possibly, to avoid string definitions:

assert all(t1==t2 for t1, t2 in zip(ts1.tables, ts2.tables) if not isinstance(t1[1], tskit.ProvenanceTable))

@hyanwong
Copy link
Member Author

Even better, we can easily check identity without the provenance timestamps having to be the same:

for t1, t2 in zip(ts1.tables, ts2.tables):
 if isinstance(t1[1], tskit.ProvenanceTable):
  assert all(t1[1].record==t2[1].record)
 else:
  assert t1==t2

@codecov
Copy link

codecov bot commented Jun 20, 2019

Codecov Report

Merging #244 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #244      +/-   ##
==========================================
+ Coverage   86.64%   86.66%   +0.01%     
==========================================
  Files          17       17              
  Lines        9437     9446       +9     
  Branches     1712     1712              
==========================================
+ Hits         8177     8186       +9     
  Misses        750      750              
  Partials      510      510
Flag Coverage Δ
#c_tests 86.66% <100%> (+0.01%) ⬆️
#python_tests 98.25% <100%> (ø) ⬆️
Impacted Files Coverage Δ
python/tskit/tables.py 99.79% <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 4d6fdf0...9fe458d. Read the comment docs.

Copy link
Member

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of any other meaning we could have for __iter__, so this seems like a sensible way of using it. I think we should use the simple definition based on generators for production code though (can use current definition for testing).

python/tskit/tables.py Show resolved Hide resolved
@hyanwong
Copy link
Member Author

hyanwong commented Jun 21, 2019

@jeromekelleher: do you think it is also worth including the table-check-without-timestamps code in tskit? e.g.

class TableCollection():
    ...
    def equal(self, tc_to_compare, ignore_provenance_timestamps=True):
        for t1, t2 in zip(self, tc_to_compare):
            if ignore_provenance_timestamps and isinstance(t1[1], tskit.ProvenanceTable):
                if all(t1[1].record!=t2[1].record):
                    return False
            else:
                if t1 != t2
                    return False
        return True

@jeromekelleher
Copy link
Member

@jeromekelleher: do you think it is also worth including the table-check-without-timestamps code in tskit? e.g.

No, to be honest. We're spending more time on making this stuff that we would save.

@hyanwong hyanwong force-pushed the tables-iterator branch 4 times, most recently from 04359b2 to e13294a Compare June 24, 2019 23:04
@jeromekelleher
Copy link
Member

Looks good, thanks and ready to merge. Can you do a quick rebase to bring it up to date please?

@hyanwong
Copy link
Member Author

Done

@jeromekelleher jeromekelleher merged commit c18ac17 into tskit-dev:master Jun 25, 2019
@hyanwong hyanwong deleted the tables-iterator branch July 1, 2019 09:17
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.

2 participants