Skip to content

Commit

Permalink
Fix memory leak in table references.
Browse files Browse the repository at this point in the history
Also simplify the class structure of Tables

Closes #2428
  • Loading branch information
jeromekelleher authored and mergify[bot] committed Jul 27, 2022
1 parent ada9596 commit 8e87bfd
Showing 1 changed file with 19 additions and 21 deletions.
40 changes: 19 additions & 21 deletions python/tskit/tables.py
Original file line number Diff line number Diff line change
Expand Up @@ -349,10 +349,9 @@ class BaseTable:
# The list of columns in the table. Must be set by subclasses.
column_names = []

def __init__(self, ll_table, row_class, **kwargs):
def __init__(self, ll_table, row_class):
self.ll_table = ll_table
self.row_class = row_class
super().__init__(**kwargs)

def _check_required_args(self, **kwargs):
for k, v in kwargs.items():
Expand Down Expand Up @@ -491,6 +490,9 @@ def __setattr__(self, name, value):
else:
object.__setattr__(self, name, value)

def _make_row(self, *args):
return self.row_class(*args)

def __getitem__(self, index):
"""
If passed an integer, return the specified row of this table, decoding metadata
Expand All @@ -512,7 +514,7 @@ def __getitem__(self, index):
index += len(self)
if index < 0 or index >= len(self):
raise IndexError("Index out of bounds")
return self.row_class(*self.ll_table.get_row(index))
return self._make_row(*self.ll_table.get_row(index))
elif isinstance(index, numbers.Number):
raise TypeError("Index must be integer, slice or iterable")
elif isinstance(index, slice):
Expand Down Expand Up @@ -699,9 +701,9 @@ def _columns_all_integer(self, *colnames):
)


class MetadataColumnMixin:
class MetadataTable(BaseTable):
"""
Mixin class for tables that have a metadata column.
Base class for tables that have a metadata column.
"""

# TODO this class has some overlap with the MetadataProvider base class
Expand All @@ -710,17 +712,13 @@ class MetadataColumnMixin:
# low-level get/set metadata schemas functionality). We should refactor
# this so we're only doing it in one place.
# https://github.com/tskit-dev/tskit/issues/1957
def __init__(self):
base_row_class = self.row_class

def row_class(*args, **kwargs):
return base_row_class(
*args, **kwargs, metadata_decoder=self.metadata_schema.decode_row
)

self.row_class = row_class
def __init__(self, ll_table, row_class):
super().__init__(ll_table, row_class)
self._update_metadata_schema_cache_from_ll()

def _make_row(self, *args):
return self.row_class(*args, metadata_decoder=self.metadata_schema.decode_row)

def packset_metadata(self, metadatas):
"""
Packs the specified list of metadata values and updates the ``metadata``
Expand Down Expand Up @@ -805,7 +803,7 @@ def getter(d, k):
return out


class IndividualTable(BaseTable, MetadataColumnMixin):
class IndividualTable(MetadataTable):
"""
A table defining the individuals in a tree sequence. Note that although
each Individual has associated nodes, reference to these is not stored in
Expand Down Expand Up @@ -1067,7 +1065,7 @@ def packset_parents(self, parents):
self.set_columns(**d)


class NodeTable(BaseTable, MetadataColumnMixin):
class NodeTable(MetadataTable):
"""
A table defining the nodes in a tree sequence. See the
:ref:`definitions <sec_node_table_definition>` for details on the columns
Expand Down Expand Up @@ -1270,7 +1268,7 @@ def append_columns(
)


class EdgeTable(BaseTable, MetadataColumnMixin):
class EdgeTable(MetadataTable):
"""
A table defining the edges in a tree sequence. See the
:ref:`definitions <sec_edge_table_definition>` for details on the columns
Expand Down Expand Up @@ -1487,7 +1485,7 @@ def squash(self):
self.ll_table.squash()


class MigrationTable(BaseTable, MetadataColumnMixin):
class MigrationTable(MetadataTable):
"""
A table defining the migrations in a tree sequence. See the
:ref:`definitions <sec_migration_table_definition>` for details on the columns
Expand Down Expand Up @@ -1716,7 +1714,7 @@ def append_columns(
)


class SiteTable(BaseTable, MetadataColumnMixin):
class SiteTable(MetadataTable):
"""
A table defining the sites in a tree sequence. See the
:ref:`definitions <sec_site_table_definition>` for details on the columns
Expand Down Expand Up @@ -1932,7 +1930,7 @@ def packset_ancestral_state(self, ancestral_states):
self.set_columns(**d)


class MutationTable(BaseTable, MetadataColumnMixin):
class MutationTable(MetadataTable):
"""
A table defining the mutations in a tree sequence. See the
:ref:`definitions <sec_mutation_table_definition>` for details on the columns
Expand Down Expand Up @@ -2200,7 +2198,7 @@ def packset_derived_state(self, derived_states):
self.set_columns(**d)


class PopulationTable(BaseTable, MetadataColumnMixin):
class PopulationTable(MetadataTable):
"""
A table defining the populations referred to in a tree sequence.
The PopulationTable stores metadata for populations that may be referred to
Expand Down

0 comments on commit 8e87bfd

Please sign in to comment.