From c00ae11a1a78bb41b84587f51437cd96baac9c1e Mon Sep 17 00:00:00 2001 From: Ryan Ly Date: Tue, 24 Oct 2023 08:41:54 -0700 Subject: [PATCH] Add `target_tables` argument to `DynamicTable.__init__` (#971) Co-authored-by: Ryan Ly Co-authored-by: Oliver Ruebel --- CHANGELOG.md | 6 ++ docs/gallery/plot_dynamictable_howto.py | 35 ++++++++++ src/hdmf/common/io/table.py | 54 +-------------- src/hdmf/common/table.py | 44 ++++++++++++ tests/unit/common/test_table.py | 91 +++++++++++++++++++++++++ 5 files changed, 177 insertions(+), 53 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dd4d97d0e..0f3986421 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # HDMF Changelog +## HDMF 3.11.0 (Upcoming) + +### Enhancements +- Added `target_tables` attribute to `DynamicTable` to allow users to specify the target table of any predefined +`DynamicTableRegion` columns of a `DynamicTable` subclass. @rly [#971](https://github.com/hdmf-dev/hdmf/pull/971) + ## HDMF 3.10.0 (October 3, 2023) Since version 3.9.1 should have been released as 3.10.0 but failed to release on PyPI and conda-forge, this release diff --git a/docs/gallery/plot_dynamictable_howto.py b/docs/gallery/plot_dynamictable_howto.py index e8832479d..7f9e39c38 100644 --- a/docs/gallery/plot_dynamictable_howto.py +++ b/docs/gallery/plot_dynamictable_howto.py @@ -318,6 +318,41 @@ columns=[dtr_idx, indexed_dtr_col], ) +############################################################################### +# Setting the target table of a DynamicTableRegion column of a DynamicTable +# ------------------------------------------------------------------------- +# A subclass of DynamicTable might have a pre-defined DynamicTableRegion column. +# To write this column correctly, the "table" attribute of the column must be set so +# that users know to what table the row index values reference. Because the target +# table could be any table, the "table" attribute must be set explicitly. There are three +# ways to do so. First, you can use the ``target_tables`` argument of the +# DynamicTable constructor as shown below. This argument +# is a dictionary mapping the name of the DynamicTableRegion column to +# the target table. Secondly, the target table can be set after the DynamicTable +# has been initialized using ``my_table.my_column.table = other_table``. Finally, +# you can create the DynamicTableRegion column and pass the ``table`` +# attribute to `DynamicTableRegion.__init__` and then pass the column to +# `DynamicTable.__init__` using the `columns` argument. However, this approach +# is not recommended for columns defined in the schema, because it is up to +# the user to ensure that the column is created in accordance with the schema. + +class SubTable(DynamicTable): + __columns__ = ( + {'name': 'dtr', 'description': 'required region', 'required': True, 'table': True}, + ) + +referenced_table = DynamicTable( + name='referenced_table', + description='an example table', +) + +sub_table = SubTable( + name='sub_table', + description='an example table', + target_tables={'dtr': referenced_table}, +) +# now the target table of the DynamicTableRegion column 'dtr' is set to `referenced_table` + ############################################################################### # Creating an expandable table # ---------------------------- diff --git a/src/hdmf/common/io/table.py b/src/hdmf/common/io/table.py index 0cde4de9e..446c613e0 100644 --- a/src/hdmf/common/io/table.py +++ b/src/hdmf/common/io/table.py @@ -2,7 +2,7 @@ from ..table import DynamicTable, VectorData, VectorIndex, DynamicTableRegion from ...build import ObjectMapper, BuildManager, CustomClassGenerator from ...spec import Spec -from ...utils import docval, getargs, popargs, AllowPositional +from ...utils import docval, getargs @register_map(DynamicTable) @@ -111,55 +111,3 @@ def post_process(cls, classdict, bases, docval_args, spec): columns = classdict.get('__columns__') if columns is not None: classdict['__columns__'] = tuple(columns) - - @classmethod - def set_init(cls, classdict, bases, docval_args, not_inherited_fields, name): - if '__columns__' not in classdict: - return - - base_init = classdict.get('__init__') - if base_init is None: # pragma: no cover - raise ValueError("Generated class dictionary is missing base __init__ method.") - - # add a specialized docval arg for __init__ for specifying targets for DTRs - docval_args_local = docval_args.copy() - target_tables_dvarg = dict( - name='target_tables', - doc=('dict mapping DynamicTableRegion column name to the table that the DTR points to. The column is ' - 'added to the table if it is not already present (i.e., when it is optional).'), - type=dict, - default=None - ) - cls._add_to_docval_args(docval_args_local, target_tables_dvarg, err_if_present=True) - - @docval(*docval_args_local, allow_positional=AllowPositional.WARNING) - def __init__(self, **kwargs): - target_tables = popargs('target_tables', kwargs) - base_init(self, **kwargs) - - # set target attribute on DTR - if target_tables: - for colname, table in target_tables.items(): - if colname not in self: # column has not yet been added (it is optional) - column_conf = None - for conf in self.__columns__: - if conf['name'] == colname: - column_conf = conf - break - if column_conf is None: - raise ValueError("'%s' is not the name of a predefined column of table %s." - % (colname, self)) - if not column_conf.get('table', False): - raise ValueError("Column '%s' must be a DynamicTableRegion to have a target table." - % colname) - self.add_column(name=column_conf['name'], - description=column_conf['description'], - index=column_conf.get('index', False), - table=True) - if isinstance(self[colname], VectorIndex): - col = self[colname].target - else: - col = self[colname] - col.table = table - - classdict['__init__'] = __init__ diff --git a/src/hdmf/common/table.py b/src/hdmf/common/table.py index e174564af..58f0470e1 100644 --- a/src/hdmf/common/table.py +++ b/src/hdmf/common/table.py @@ -292,9 +292,15 @@ def __gather_columns(cls, name, bases, classdict): {'name': 'colnames', 'type': 'array_data', 'doc': 'the ordered names of the columns in this table. columns must also be provided.', 'default': None}, + {'name': 'target_tables', + 'doc': ('dict mapping DynamicTableRegion column name to the table that the DTR points to. The column is ' + 'added to the table if it is not already present (i.e., when it is optional).'), + 'type': dict, + 'default': None}, allow_positional=AllowPositional.WARNING) def __init__(self, **kwargs): # noqa: C901 id, columns, desc, colnames = popargs('id', 'columns', 'description', 'colnames', kwargs) + target_tables = popargs('target_tables', kwargs) super().__init__(**kwargs) self.description = desc @@ -468,6 +474,10 @@ def __init__(self, **kwargs): # noqa: C901 self.__colids = {name: i + 1 for i, name in enumerate(self.colnames)} self._init_class_columns() + if target_tables: + self._set_dtr_targets(target_tables) + + def __set_table_attr(self, col): if hasattr(self, col.name) and col.name not in self.__uninit_cols: msg = ("An attribute '%s' already exists on %s '%s' so this column cannot be accessed as an attribute, " @@ -516,6 +526,40 @@ def _init_class_columns(self): self.__uninit_cols[col['name'] + '_elements'] = col setattr(self, col['name'] + '_elements', None) + def _set_dtr_targets(self, target_tables: dict): + """Set the target tables for DynamicTableRegion columns. + + If a column is not yet initialized, it is initialized with the target table. + """ + for colname, table in target_tables.items(): + if colname not in self: # column has not yet been added (it is optional) + column_conf = None + for conf in self.__columns__: + if conf['name'] == colname: + column_conf = conf + break + if column_conf is None: + raise ValueError("'%s' is not the name of a predefined column of table %s." + % (colname, self)) + if not column_conf.get('table', False): + raise ValueError("Column '%s' must be a DynamicTableRegion to have a target table." + % colname) + self.add_column(name=column_conf['name'], + description=column_conf['description'], + index=column_conf.get('index', False), + table=True) + if isinstance(self[colname], VectorIndex): + col = self[colname].target + else: + col = self[colname] + if not isinstance(col, DynamicTableRegion): + raise ValueError("Column '%s' must be a DynamicTableRegion to have a target table." % colname) + # if columns are passed in, then the "table" attribute may have already been set + if col.table is not None and col.table is not table: + raise ValueError("Column '%s' already has a target table that is not the passed table." % colname) + if col.table is None: + col.table = table + @staticmethod def __build_columns(columns, df=None): """ diff --git a/tests/unit/common/test_table.py b/tests/unit/common/test_table.py index c398981d4..88f8ca07b 100644 --- a/tests/unit/common/test_table.py +++ b/tests/unit/common/test_table.py @@ -1598,6 +1598,97 @@ def test_init_columns_add_dup_column(self): with self.assertRaisesWith(ValueError, msg): SubTable(name='subtable', description='subtable description', columns=[col1_ind, col1]) + def test_no_set_target_tables(self): + """Test that the target table of a predefined DTR column is None.""" + table = SubTable(name='subtable', description='subtable description') + self.assertIsNone(table.col5.table) + + def test_set_target_tables(self): + """Test setting target tables for predefined DTR columns.""" + table1 = SubTable(name='subtable1', description='subtable description') + table2 = SubTable( + name='subtable2', + description='subtable description', + target_tables={ + 'col5': table1, + 'col6': table1, + 'col7': table1, + 'col8': table1, + }, + ) + self.assertIs(table2.col5.table, table1) + self.assertIs(table2.col6.table, table1) + self.assertIs(table2.col7.table, table1) + self.assertIs(table2.col8.table, table1) + + def test_set_target_tables_unknown_col(self): + """Test setting target tables for unknown columns.""" + table1 = SubTable(name='subtable1', description='subtable description') + msg = r"'bad_col' is not the name of a predefined column of table subtable2 .*" + with self.assertRaisesRegex(ValueError, msg): + SubTable( + name='subtable2', + description='subtable description', + target_tables={ + 'bad_col': table1, + }, + ) + + def test_set_target_tables_bad_init_col(self): + """Test setting target tables for predefined, required non-DTR columns.""" + table1 = SubTable(name='subtable1', description='subtable description') + msg = "Column 'col1' must be a DynamicTableRegion to have a target table." + with self.assertRaisesWith(ValueError, msg): + SubTable( + name='subtable2', + description='subtable description', + target_tables={ + 'col1': table1, + }, + ) + + def test_set_target_tables_bad_opt_col(self): + """Test setting target tables for predefined, optional non-DTR columns.""" + table1 = SubTable(name='subtable1', description='subtable description') + msg = "Column 'col2' must be a DynamicTableRegion to have a target table." + with self.assertRaisesWith(ValueError, msg): + SubTable( + name='subtable2', + description='subtable description', + target_tables={ + 'col2': table1, + }, + ) + + def test_set_target_tables_existing_col_mismatch(self): + """Test setting target tables for an existing DTR column with a mismatched, existing target table.""" + table1 = SubTable(name='subtable1', description='subtable description') + table2 = SubTable(name='subtable2', description='subtable description') + dtr = DynamicTableRegion(name='dtr', data=[], description='desc', table=table1) + msg = "Column 'dtr' already has a target table that is not the passed table." + with self.assertRaisesWith(ValueError, msg): + SubTable( + name='subtable3', + description='subtable description', + columns=[dtr], + target_tables={ + 'dtr': table2, + }, + ) + + def test_set_target_tables_existing_col_match(self): + """Test setting target tables for an existing DTR column with a matching, existing target table.""" + table1 = SubTable(name='subtable1', description='subtable description') + dtr = DynamicTableRegion(name='dtr', data=[], description='desc', table=table1) + SubTable( + name='subtable2', + description='subtable description', + columns=[dtr], + target_tables={ + 'dtr': table1, + }, + ) + class TestEnumData(TestCase):