Add fancy indexing to tables #1348
Conversation
benjeffery
force-pushed
the
table-indexing
branch
from
7875fc3 to
af9271d
Compare
Codecov Report
@@ Coverage Diff @@
## main #1348 +/- ##
=======================================
Coverage 93.89% 93.89%
=======================================
Files 26 26
Lines 22545 22561 +16
Branches 1066 1071 +5
=======================================
+ Hits 21168 21184 +16
Misses 1343 1343
Partials 34 34
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
benjeffery
force-pushed
the
table-indexing
branch
from
af9271d to
0d5d074
Compare
| if hasattr(table1, "metadata_schema"): | ||
| assert table1.metadata_schema == tskit.MetadataSchema(None) | ||
| table2 = table1.copy() | ||
| def test_metadata_schema(self, table_5row): |
There was a problem hiding this comment.
I like the abstract idea of fixtures (i.e., not making up test data over and over again), but I have to say I find this idiom of changing all the variables in a test to (effectively) a global variable really nasty. These tests are now objectively less readable that they were before. Wouldn't a parametrize with a set of locally-defined example tables be better?
There was a problem hiding this comment.
Hmm, I see, you've made it a 5row table so that you know in later tests that you can do things like table[:4] or whatever. Maybe disregard above comment then.
There was a problem hiding this comment.
The main change is moving the fixture to where it can be used by AssertEqualsMixin and FancyIndexingMixin. I agree the non-locality of fixtures can be a readability issue, but generally think it is worth the trade off.
python/tskit/tables.py
Outdated
| if it is present. Supports negative indexing, e.g. ``table[-5]``. | ||
| If passed a slice, iterable or array return a new table containing the specified | ||
| rows. Similar to numpy fancy indexing, if the array or iterables contains | ||
| booleans then in acts as a mask, returning those rows for which are True. |
There was a problem hiding this comment.
The sentence isn't quite grammatical, something like "returning those rows for which the mask is True."
| """ | ||
|
|
||
| if isinstance(index, numbers.Integral): | ||
| # Single row by integer |
There was a problem hiding this comment.
Out of curiosity, could you do index = slice(index).indices(len(self))[0]?
More obscure, I guess, but saves a bit of fiddly code?
There was a problem hiding this comment.
Unfortunately not:
>>> slice(5).indices(10)
(0, 5, 1)
benjeffery
force-pushed
the
table-indexing
branch
from
0d5d074 to
457d50d
Compare
Fixes #1221