Add ref seq equals #2085
Add ref seq equals #2085
Conversation
benjeffery
force-pushed
the
refseq_eq
branch
2 times, most recently
from
3a5408b to
3054a47
Compare
Codecov Report
@@ Coverage Diff @@
## main #2085 +/- ##
=======================================
Coverage 93.38% 93.39%
=======================================
Files 27 27
Lines 25561 25569 +8
Branches 1159 1159
=======================================
+ Hits 23870 23879 +9
Misses 1660 1660
+ Partials 31 30 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
jeromekelleher
left a comment
jeromekelleher
left a comment
There was a problem hiding this comment.
Looks good, a few comments.
What do we do with the relationship between assert_equals and equals elsewhere?
|
|
||
| class TestAssertEquals: | ||
| class TestEquals: | ||
| def test_success_self(self, ts_fixture): |
There was a problem hiding this comment.
I guess we should rename the rest of the methods here to reflect the change in the class name. "success" doesn't make sense any more.
| ("metadata_schema", tskit.MetadataSchema(None)), | ||
| ], | ||
| ) | ||
| def test_fails_different(self, ts_fixture, attr, val): |
There was a problem hiding this comment.
This test doesn't really make sense any more - it doesn't "fail" now because we're not doing asserts. It's also too complicated IMO - we should be aiming to "test one thing" in these tests, and that one thing should be reflected in the name.
So, I think it'd be better if we split this into two tests, one for different_not_equal and different_but_ignore or something? As a rule, I think doing parametrize and using ifs on the values should be an anti-pattern.
python/tskit/metadata.py
Outdated
| def nbytes(self) -> int: | ||
| return len(self._ll_object.metadata) + len(self._ll_object.metadata_schema) | ||
|
|
||
| def equals(self, other): |
There was a problem hiding this comment.
Might as well include the -> bool annotation
python/tskit/metadata.py
Outdated
| def nbytes(self) -> int: | ||
| return len(self._ll_object.metadata) + len(self._ll_object.metadata_schema) | ||
|
|
||
| def equals(self, other): |
There was a problem hiding this comment.
Should we use
try:
self.assert_equals(...)
return True
except AssertionError:
return Falsehere so that we're not defining equality twice? I think we've done that elsewhere in non-performance critical cases?
python/tskit/tables.py
Outdated
| def __eq__(self, other): | ||
| return self.equals(other) | ||
|
|
||
| def equals(self, other, ignore_metadata=False, ignore_url=False): |
There was a problem hiding this comment.
What's the motivation for ignore_url here? I don't think there's much point in adding this in yet if we don't have a specific application because we might end up having to row-back on it when we do start using the URL field.
There was a problem hiding this comment.
As it is similar to metadata a put it in, but yeah makes sense to wait till we work out what it is doing.
In almost all the others we are calling a low-level method in |
|
@jeromekelleher comments addressed! Thanks. |
jeromekelleher
left a comment
jeromekelleher
left a comment
There was a problem hiding this comment.
LGTM! One thing to clear up before merging.
python/tskit/metadata.py
Outdated
| def nbytes(self) -> int: | ||
| return len(self._ll_object.metadata) + len(self._ll_object.metadata_schema) | ||
|
|
||
| def equals(self, other) -> bool: |
There was a problem hiding this comment.
Looks like we're not calling this any more -simplest to just delete it I guess?
2 participants
Fixes #2063