Raise an error when messing with coords if there is a reference_sequence #3210
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3210 +/- ##
=======================================
Coverage 89.63% 89.63%
=======================================
Files 28 28
Lines 32000 32004 +4
Branches 5877 5879 +2
=======================================
+ Hits 28684 28688 +4
Misses 1886 1886
Partials 1430 1430
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
I added an extra commit to this as I was documenting the error returns, as it was easier to make the trim() commands do the right thing with migrations than figure out how to word the description of when the error message was triggered! |
benjeffery
left a comment
benjeffery
left a comment
There was a problem hiding this comment.
Couple of comments, needs a changelog.
python/tskit/tables.py
Outdated
| :param bool record_provenance: If ``True``, add details of this operation | ||
| to the provenance table in this TableCollection. (Default: ``True``). | ||
| :raises ValueError: If the TableCollection has no edges, has a reference sequence | ||
| or has migrations that extend into the trimmed regions. |
There was a problem hiding this comment.
This line is no longer true?
|
|
||
| def test_failure_with_migrations(self): | ||
| def test_migration_trimming(self): | ||
| # All trim functions fail if migrations extend further than rightmost or |
There was a problem hiding this comment.
This comment is now orphaned.
|
These tests for migrations don't look very comprehensive to me - can we switch back to raising an error if we're not going to put the effort in to doing things properly? It is much better to not implement something (particularly if it's very unlikely to be used) that to do it badly and introduce bugs for others to trace down and fix later. |
I can revert the second commit. It seemed easier to deal with this than document the exceptions, that's all. And I assume we will fix this post-1.0? |
We don't need to document the exceptions, that's what the error message is for. This is a remote corner of the API that like as not no one will ever use. If they do, the will hit the error and they can open an issue asking for it to be implemented. A poorly tested implementation is much worse than this. |
|
OK, reverted to first commit, removed description of exceptions (I thought we had started wanting to do this though?) and added changelog. |
|
I don't think we need to get too particular about writing documentation for every corner case and combination of features that are and are not implemented together - a descriptive error message is much more helpful. |
3 participants
Fixes #2091