[codex] Improve dynamic table validation and colnames sync by ehennestad · Pull Request #818 · NeurodataWithoutBorders/matnwb

ehennestad · 2026-04-17T14:30:28Z

Motivation

This PR fixes gaps in DynamicTable validation and synchronization around colnames.

Changes:

Ensure the colnames property contains unique elements
Validates that materialized table columns are listed in colnames
Updates generated setters for schema-defined DynamicTable columns so direct assignments keep colnames in sync.
Adds re-check of DynamicTable configuration during nwbExport to ensure dynamic tables are still valid when writing.

How to test the behavior?

nwbtest('Name','tests.unit.dynamicTableTest')
nwbtest('Name','tests.system.DynamicTableTest')

Example 1: Adding duplicate colnames

vectorDataA = types.hdmf_common.VectorData('description', 'column a', 'data', 1);

dynamicTable = types.hdmf_common.DynamicTable( ...
    'description', 'example', ...
    'colnames', {'columnA', 'columnA'}, ...
    'columnA', vectorDataA );

Before:

colnames: {columnA, columnA}

main accepted duplicate colnames.

After:

Error using types.util.dynamictable.validateColnames
Column names in `colnames` must be unique.

The feature branch rejects duplicate colnames.

Example 2: Adding wrong colname

vectorDataA = types.hdmf_common.VectorData('description', 'column a', 'data', 1);

dynamicTable = types.hdmf_common.DynamicTable( ...
    'description', 'example', ...
    'colnames', {'columnB'}, ...
    'columnA', vectorDataA );

Before:

colnames: {columnB}

main allowed a materialized column to be missing from colnames.

After:

Error using types.util.dynamictable.checkConfig
All materialized DynamicTable columns must be listed in `colnames`.
Missing from `colnames`: columnA

The feature branch detects the mismatch immediately.

Example 3: Adding column w/o colnames automatically updates colnames

timeIntervals = types.core.TimeIntervals( ...
    'description', 'example');

timeIntervals.start_time = types.hdmf_common.VectorData('description', 'start time', 'data', 1);

timeIntervals

Before:

colnames: {}

On main, direct assignment does not sync colnames.

After:

colnames: {start_time}

On the feature branch, direct assignment updates colnames automatically.

Example 4: nwbExport fails for invalid table

vectorDataA = types.hdmf_common.VectorData('description', 'column a', 'data', 1);

dynamicTable = types.hdmf_common.DynamicTable( ...
    'description', 'example', ...
    'colnames', {'columnA'}, ...
    'columnA', vectorDataA );
dynamicTable.colnames = {'columnB', 'columnC'};

nwb = tests.factory.NWBFile();
nwb.acquisition.set('DynamicTable', dynamicTable);
nwbExport(nwb, 'temp_dynamic_table_check_export')

Before:

Export succeeded.

main allowed export to proceed even after colnames drifted out of sync.

After:

Error using nwbExport
The following error was caught when exporting type "types.hdmf_common.DynamicTable" at file location "/acquisition/DynamicTable":
All materialized DynamicTable columns must be listed in `colnames`.
Missing from `colnames`: columnA

The feature branch re-validates DynamicTable configuration during export.

Checklist

Have you ensured the PR description clearly describes the problem and solutions?
Have you checked to ensure that there aren't other open or previously closed Pull Requests for the same change?
If this PR fixes an issue, is the first line of the PR description fix #XX where XX is the issue number?

🤖 Generated with Codex

Fix broken test

Copilot

Pull request overview

This PR tightens DynamicTable invariants around colnames by adding normalization/validation, syncing colnames when schema-defined column properties are assigned, and re-checking table configuration during export to catch drift.

Changes:

Added validateColnames (uniqueness + normalization) and syncNamedColumn utilities under types.util.dynamictable.
Updated checkConfig to validate colnames and detect materialized columns missing from colnames.
Updated class generation to inject validator/setter hooks (and added/updated tests) so direct assignments keep colnames consistent and export re-validates constraints.

Reviewed changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
+types/+util/+dynamictable/validateColnames.m	New helper to normalize and enforce uniqueness of `colnames`.
+types/+util/+dynamictable/syncNamedColumn.m	New helper to ensure schema-defined columns are registered in `colnames` on assignment.
+types/+util/+dynamictable/checkConfig.m	Extends DynamicTable config validation (now checks for missing detected columns and normalizes `colnames`).
+types/+hdmf_common/DynamicTable.m	Uses `validateColnames` in `validate_colnames` and adds `checkCustomConstraint` to re-run `checkConfig` (e.g., on export).
+types/+core/Units.m	Adds post-set hooks to sync schema-defined columns into `colnames` on direct assignment.
+types/+core/TimeIntervals.m	Adds post-set hooks to sync schema-defined columns into `colnames` on direct assignment.
+types/+core/SweepTable.m	Adds post-set hooks to sync schema-defined columns into `colnames` on direct assignment.
+types/+core/SimultaneousRecordingsTable.m	Adds post-set hooks to sync schema-defined columns into `colnames` on direct assignment.
+types/+core/SequentialRecordingsTable.m	Adds post-set hooks to sync schema-defined columns into `colnames` on direct assignment.
+types/+core/RepetitionsTable.m	Adds post-set hooks to sync schema-defined columns into `colnames` on direct assignment.
+types/+core/PlaneSegmentation.m	Adds post-set hooks to sync schema-defined columns into `colnames` on direct assignment.
+types/+core/IntracellularStimuliTable.m	Adds post-set hooks to sync schema-defined columns into `colnames` on direct assignment.
+types/+core/IntracellularResponsesTable.m	Adds post-set hooks to sync schema-defined columns into `colnames` on direct assignment.
+types/+core/IntracellularElectrodesTable.m	Adds post-set hooks to sync schema-defined columns into `colnames` on direct assignment.
+types/+core/FrequencyBandsTable.m	Adds post-set hooks to sync schema-defined columns into `colnames` on direct assignment.
+types/+core/ExperimentalConditionsTable.m	Adds post-set hooks to sync schema-defined columns into `colnames` on direct assignment.
+types/+core/ElectrodesTable.m	Adds post-set hooks to sync schema-defined columns into `colnames` on direct assignment.
+tests/+unit/dynamicTableTest.m	Updates/extends tests for duplicate `colnames`, direct-assignment syncing, and export-time drift detection.
+file/isDynamicTableDescendant.m	New generator helper to detect DynamicTable inheritance.
+file/getPropertyHooks.m	New generator hook registry for custom validator/setter behavior.
+file/fillValidators.m	Injects additional validator lines via generator hooks.
+file/fillSetters.m	Adds optional post-set hook generation via generator hooks.
+file/fillCustomConstraint.m	Adds custom constraint generation for `DynamicTable` to re-check config.
+file/fillConstructor.m	Uses new `file.isDynamicTableDescendant` helper when injecting DynamicTable validation into constructors.
+file/fillClass.m	Passes type/namespace into `fillSetters` for hook-aware setter generation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-20T15:38:25Z

+    colnames = cleanColumnNames(colnames);
+    if ischar(colnames)
+        colnames = {colnames};
+    end
+
+    assert(iscellstr(colnames), ...
+        'NWB:DynamicTable:InvalidColumnNames', ...
+        'Column names must be a cell array of character vectors.');
+


validate_colnames accepts MATLAB string arrays because types.util.checkDtype(...,'char',val) allows isstring(val), but validateColnames later asserts iscellstr(colnames)/ischar(colnames) and will therefore reject string input. Consider normalizing string (and cell arrays of strings) to cellstr early (e.g., colnames = cellstr(colnames) when isstring(colnames)), so the validator is consistent with checkDtype and common MATLAB calling patterns.

@copilot apply changes based on this feedback

Copilot · 2026-04-20T15:38:26Z

+    if ischar(colnames)
+        colnames = {colnames};
+    end
+
+    assert(iscellstr(colnames), ...
+        'NWB:DynamicTable:InvalidColumnNames', ...
+        'Column names must be a cell array of character vectors.');


validateColnames will also fail for an N-by-M char array (multiple column names stored as padded rows), because it wraps any char input into a 1-element cell and then requires iscellstr. If colnames can be a char matrix (e.g., from some HDF5 string array reads or user input), consider converting multi-row char arrays via cellstr(colnames) before validating uniqueness.

I don't think char matrix should be supported

Copilot · 2026-04-20T15:38:26Z

+        || isa(value, 'types.core.VectorData');
+    isVectorIndex = isa(value, 'types.hdmf_common.VectorIndex') ...
+        || isa(value, 'types.core.VectorIndex');
+    tf = ~isempty(value) && isVectorData && ~isVectorIndex;


isMaterializedColumn currently treats any non-empty VectorData object as “materialized”, regardless of whether its .data is empty / zero-length (the previous implementation in getDetectedColumnNames explicitly skipped empty Vector.data for DataStub, DataPipe, and in-memory arrays). If the intent is to validate only columns with actual row data, reintroduce the data-emptiness check (or rename the helper/message to reflect the new semantics).

Suggested change

tf = ~isempty(value) && isVectorData && ~isVectorIndex;

hasData = false;

if ~isempty(value) && isVectorData && ~isVectorIndex

hasData = ~isempty(value.data);

end

tf = ~isempty(value) && isVectorData && ~isVectorIndex && hasData;

I think the current implementation is correct. Presence of VectorData should be treated as a materialised column. If data is empty, an error should be thrown later in checkConfig if other columns in the same table has a non-zero length. If the property is dropped here because data is empty, the column height check would not flag this empty column as a problem later.

Agent-Logs-Url: https://github.com/NeurodataWithoutBorders/matnwb/sessions/f5f16c03-9489-4eee-8c7f-127005b690ce Co-authored-by: ehennestad <17237719+ehennestad@users.noreply.github.com>

…/NeurodataWithoutBorders/matnwb into improve-dynamic-table-validation

codecov · 2026-04-21T12:10:26Z

Codecov Report

❌ Patch coverage is 97.41379% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.25%. Comparing base (f6a4dbc) to head (effd91b).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
+types/+util/+dynamictable/validateColnames.m	88.88%	2 Missing ⚠️
+types/+util/+dynamictable/checkConfig.m	95.45%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #818      +/-   ##
==========================================
- Coverage   95.35%   95.25%   -0.10%     
==========================================
  Files         205      209       +4     
  Lines        7430     7508      +78     
==========================================
+ Hits         7085     7152      +67     
- Misses        345      356      +11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ehennestad added 3 commits April 17, 2026 16:24

First pass by codex

ecfb6cd

Update dynamicTableTest.m

4457153

Fix broken test

Merge branch 'main' into improve-dynamic-table-validation

baa3aec

ehennestad requested a review from Copilot April 20, 2026 15:31

Copilot started reviewing on behalf of ehennestad April 20, 2026 15:31 View session

Copilot AI reviewed Apr 20, 2026

View reviewed changes

Copilot started work on behalf of ehennestad April 21, 2026 10:30 View session

Fix checkConfig to apply ignoreList before missingColumnNames assertion

8b1b01d

Agent-Logs-Url: https://github.com/NeurodataWithoutBorders/matnwb/sessions/f5f16c03-9489-4eee-8c7f-127005b690ce Co-authored-by: ehennestad <17237719+ehennestad@users.noreply.github.com>

Copilot finished work on behalf of ehennestad April 21, 2026 10:35

ehennestad added 3 commits April 21, 2026 13:00

validateColnames should accept string values

c03ea42

Merge branch 'improve-dynamic-table-validation' of https://github.com…

830902a

…/NeurodataWithoutBorders/matnwb into improve-dynamic-table-validation

Update docstring in checkConfig to reflect recent changes

e1c1da7

ehennestad marked this pull request as ready for review April 21, 2026 11:57

ehennestad enabled auto-merge April 21, 2026 11:57

Simplify type normalization of colnames in validateColnames

1bb0c09

bendichter self-requested a review April 21, 2026 14:28

Merge branch 'main' into improve-dynamic-table-validation

63279a6

bendichter approved these changes Apr 21, 2026

View reviewed changes

Merge branch 'main' into improve-dynamic-table-validation

effd91b

ehennestad added this pull request to the merge queue Apr 21, 2026

Merged via the queue into main with commit faf3801 Apr 21, 2026
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[codex] Improve dynamic table validation and colnames sync#818

[codex] Improve dynamic table validation and colnames sync#818
ehennestad merged 10 commits intomainfrom
improve-dynamic-table-validation

ehennestad commented Apr 17, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 20, 2026

Uh oh!

ehennestad Apr 21, 2026

Uh oh!

Copilot AI Apr 20, 2026

Uh oh!

ehennestad Apr 21, 2026

Uh oh!

Uh oh!

Copilot AI Apr 20, 2026

Uh oh!

ehennestad Apr 21, 2026

Uh oh!

Uh oh!

codecov Bot commented Apr 21, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

-    tf = ~isempty(value) && isVectorData && ~isVectorIndex;
+    hasData = false;
+    if ~isempty(value) && isVectorData && ~isVectorIndex
+        hasData = ~isempty(value.data);
+    end
+    tf = ~isempty(value) && isVectorData && ~isVectorIndex && hasData;

Conversation

ehennestad commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

How to test the behavior?

Example 1: Adding duplicate colnames

Example 2: Adding wrong colname

Example 3: Adding column w/o colnames automatically updates colnames

Example 4: nwbExport fails for invalid table

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

ehennestad Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

ehennestad Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

ehennestad Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov Bot commented Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ehennestad commented Apr 17, 2026 •

edited

Loading

codecov Bot commented Apr 21, 2026 •

edited

Loading