Skip to content

Check DynamicTable columns against __columns__ - #1557

Open
adityasingh2400 wants to merge 3 commits into
hdmf-dev:devfrom
adityasingh2400:fix-1553-dynamictable-columns
Open

Check DynamicTable columns against __columns__#1557
adityasingh2400 wants to merge 3 commits into
hdmf-dev:devfrom
adityasingh2400:fix-1553-dynamictable-columns

Conversation

@adityasingh2400

Copy link
Copy Markdown

Motivation

Fix #1553

DynamicTable.__init__ did not check the columns passed via columns= against __columns__, so a column whose name matched a predefined column could be passed in with the wrong class and was silently accepted. This resolves the long-standing TODO at the top of the columns processing block. In PyNWB this is how an EventsTable ends up with a plain VectorData named timestamp where the schema requires a TimestampVectorData, producing a file that fails NWB validation.

add_column had the same root cause from the other direction. col_cls was defaulted to VectorData before the predefined spec was consulted, so a caller who never passed col_cls got a plain VectorData for a predefined typed column, plus a warning that blamed an argument they did not supply.

The fix

The constructor now checks each passed column against __columns__ and warns on a class mismatch. This follows the "warn now, error in a future version of HDMF" wording that add_column already uses, so the two paths report the same class of problem the same way. The required class comes from the spec's class key, falling back to DynamicTableRegion for table: True and EnumData for enum: True, which covers the table region case named in the original TODO. The check is isinstance, so a subclass of the required class satisfies the spec, matching how a subtype satisfies data_type_inc.

add_column now consults the predefined spec before falling back to VectorData. A caller who does not pass col_cls gets the class the spec asks for, and the mismatch warning fires only when a col_cls that actually conflicts was passed. An explicit table= or enum= argument still wins, as before.

One existing test, test_add_opt_column_mismatched_col_cls, asserted the old behavior of the second case: it called add_column(name='col10', ...) with no col_cls and expected both the misleading warning and a plain VectorData, even though col10 is predefined with class=EnumData. That test now passes an actually-conflicting col_cls=VectorData, which is what its name describes, and the no-col_cls case is covered by a new test_add_opt_column_uses_spec_col_cls.

How to test the behavior?

class TypedVectorData(VectorData):
    """stand-in for pynwb's TimestampVectorData"""

class MyTable(DynamicTable):
    __columns__ = (
        {'name': 'req', 'description': 'required typed col', 'required': True, 'class': TypedVectorData},
        {'name': 'opt', 'description': 'optional typed col', 'class': TypedVectorData},
    )

# before: no error, no warning.  after: UserWarning naming the class mismatch
MyTable(name='t', description='d', columns=[VectorData(name='req', description='p', data=[1, 2, 3])])

# before: VectorData plus a warning about a col_cls the caller never passed.  after: TypedVectorData, no warning
t = MyTable(name='t', description='d', columns=[TypedVectorData(name='req', description='p', data=[1, 2, 3])])
t.add_column(name='opt', description='o', data=[4, 5, 6])

Five tests were added or updated in TestDynamicTableClassColumns. With the source change reverted and the tests kept, three fail:

FAILED tests/unit/common/test_table.py::TestDynamicTableClassColumns::test_add_opt_column_uses_spec_col_cls
FAILED tests/unit/common/test_table.py::TestDynamicTableClassColumns::test_init_columns_mismatched_col_cls
FAILED tests/unit/common/test_table.py::TestDynamicTableClassColumns::test_init_columns_mismatched_table_region
3 failed, 31 passed

With the fix applied all 34 tests in that class pass, and the full suite matches the baseline: 1869 passed, 118 skipped, 1 xfailed, 919 subtests passed.

Checklist

  • Did you update CHANGELOG.md with your changes?
  • Does the PR clearly describe the problem and the solution?
  • Have you reviewed our Contributing Guide?
  • Does the PR use "Fix #XXX" notation to tell GitHub to close the relevant issue numbered XXX when the PR is merged?

DynamicTable.__init__ did not check the columns passed via 'columns' against
__columns__, so a column whose name matched a predefined column could be passed
with the wrong class and was silently accepted. This resolves the TODO at the
top of the columns processing block.

add_column had the same root cause from the other direction: col_cls was
defaulted to VectorData before the predefined spec was consulted, so a caller
who never passed col_cls got a plain VectorData for a predefined typed column
plus a warning that blamed an argument they did not supply. The predefined
class is now used when the caller does not pass col_cls, and the warning is
emitted only for a col_cls that actually conflicts.

Fix hdmf-dev#1553
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DynamicTable does not check constructor-passed columns against __columns__

1 participant