Refactor constructor generation by ehennestad · Pull Request #786 · NeurodataWithoutBorders/matnwb

ehennestad · 2026-01-27T20:29:12Z

Motivation

Refactor constructor generator logic for neurodata type classes to facilitate modular construction of target class specific checks and setup routines.

Changes

Fixes a bug where the types.util.dynamictable.checkConfig function was not invoked in the DynamicTable constructor.
Adds warning suppression for an intended code pattern that raises a MATLAB Code Analyzer warning.
Moves call to obj.setupHasUnnamedGroupsMixin() from main constructor body to conditional block that only runs for specific class constructor (not superclass constructor)
Fixes a bug in tests.unit.dynamicTableTest (unrelated, but discovered because of fix in pt 1).

How to test the behavior?

N/A

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?

codecov · 2026-01-27T21:17:05Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.50%. Comparing base (bd167f2) to head (b9da70e).
⚠️ Report is 1 commits behind head on decouple-ci-testing-from-pynwb-dev.

Additional details and impacted files

@@                          Coverage Diff                           @@
##           decouple-ci-testing-from-pynwb-dev     #786      +/-   ##
======================================================================
- Coverage                               95.50%   95.50%   -0.01%     
======================================================================
  Files                                     191      191              
  Lines                                    6946     6944       -2     
======================================================================
- Hits                                     6634     6632       -2     
  Misses                                    312      312

☔ 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.

Copilot

Pull request overview

This pull request refactors the constructor generation logic for neurodata type classes to enable modular construction of target class-specific validation and setup routines. The primary changes consolidate constructor validation/setup code into a single conditional block and fix a critical bug where DynamicTable validation was not being executed.

Changes:

Refactors constructor generation to consolidate validation and setup code into a single conditional block per class
Fixes bug where types.util.dynamictable.checkConfig was not invoked in DynamicTable constructors
Adds %#ok<STISA> warning suppression for intentional use of strcmp(class(obj), ...) pattern

Reviewed changes

Copilot reviewed 94 out of 94 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
+file/fillConstructor.m	Refactors constructor generation logic to build a single conditional validation block; removes old `fillCheck` function and adds new `isDynamicTableDescendant` helper function
+tests/+unit/dynamicTableTest.m	Updates test to properly construct DynamicTable with VectorData objects instead of raw data arrays
+types/+hdmf_experimental/HERD.m	Adds explanatory comment and warning suppression for class comparison check
+types/+hdmf_experimental/EnumData.m	Adds explanatory comment and warning suppression for class comparison check
+types/+hdmf_common/VectorIndex.m	Adds explanatory comment and warning suppression for class comparison check
+types/+hdmf_common/VectorData.m	Adds explanatory comment and warning suppression for class comparison check
+types/+hdmf_common/SimpleMultiContainer.m	Moves setupHasUnnamedGroupsMixin() into conditional block; adds comment and warning suppression
+types/+hdmf_common/ElementIdentifiers.m	Adds explanatory comment and warning suppression for class comparison check
+types/+hdmf_common/DynamicTableRegion.m	Adds explanatory comment and warning suppression for class comparison check
+types/+hdmf_common/DynamicTable.m	Adds checkConfig call to fix validation bug; adds comment and warning suppression
+types/+hdmf_common/Data.m	Adds explanatory comment and warning suppression for class comparison check
+types/+hdmf_common/Container.m	Adds explanatory comment and warning suppression for class comparison check
+types/+hdmf_common/CSRMatrix.m	Adds explanatory comment and warning suppression for class comparison check
+types/+hdmf_common/AlignedDynamicTable.m	Consolidates duplicate conditional blocks; moves setupHasUnnamedGroupsMixin() into block; adds comment and warning suppression
+types/+core/*.m (70+ files)	Consistently applies refactored constructor pattern with comments, warning suppression, and proper placement of validation/setup code

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

- Moved mixin setup and DynamicTable validation logic from fillBody and fillCheck into the main constructor generation flow. - Introduced isDynamicTableDescendent helper for cleaner ancestry checks. - Removed the now-redundant fillCheck function.

Fix warning suppression in sprintf statement (add escape char)

Fixes bugs in test

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update TutorialTest.m * Support DataPipe in verifyContainerEqual Extend the test helper to treat types.untyped.DataPipe like DataStub by calling load() on actualValue before comparing. * Rename SKIP_PYNWB var and update CI/tests Replace the old SKIP_PYNWB_COMPATIBILITY_TEST_FOR_TUTORIALS env var with SKIP_PYNWB_TESTS across tests and nwbtest.m, and update +tests/nwbtest.default.env. Update CI workflow matrix keys and usage in prepare_release.yml, run_tests.yml, and configurations/matlab_release_matrix_strategy.yml to use the new key (matrix.skip-pynwb-tests) and wire the MATLAB setenv accordingly. Move conditional installation of pynwb into the workflows (install pynwb only when skip-pynwb-tests == '0') and remove the direct git+ dependency from +tests/requirements.txt (update comment to reflect conditional installs). Also add TestTags = {'UsesPython'} to PyNWBIOTest. These changes centralize the CI control of pynwb installation and standardize the skip variable name. * debug readthedocs failed build checking if this class attribute trips up docs build * Update PyNWBIOTest.m Move TestTags to methods blocks. Check if that fixes issue with sphinx build of docs * Add subject section to domain tutorials * Update generated html- and m-files * Update electrode/imaging plane location to valid term in tutorials * Update inspectNwbFile.m * Update inspectNwbFile.m * Make subject ids/descriptions consistent * Decouple stable CI from pynwb/nwbinspector dev branches Install pynwb and nwbinspector from PyPI stable releases in the main test workflow so CI only fails due to matnwb regressions. Add a separate weekly workflow that tests against the dev branches to catch upstream incompatibilities early without blocking PRs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Update requirements.txt * Update nwbtest.default.env Added more details in comments Removed unused vars NWB_TEST_DEBUG, GITHUB_TOKEN Added PYNWB_REPO_DIR * Updated test workflows to check out pynwb in the runner * Refactor PynwbTutorialTest to use pre-cloned pynwb repo Replace custom venv/download/GitHub API infrastructure with a simple approach: read tutorial files from PYNWB_REPO_DIR env var pointing to a pre-cloned pynwb repo, and run them against the system Python. The CI workflows are now responsible for cloning the repo and setting the env var. TestTags = {'UsesPython'} added so the tag selector in the dev workflow picks up these tests, and SKIP_PYNWB_TESTS correctly excludes them on older MATLAB releases. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * change checkout location for pynwb * Add test suite README for new developers Covers prerequisites, running tests via nwbtest() and the MATLAB unit testing framework, Python dependency setup, environment variable configuration, and test authoring conventions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix pynwb repo checkout path in CI workflows actions/checkout requires path to be within the workspace directory. Use path: pynwb-repo (inside workspace) instead of ../pynwb-repo. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Update untypedSetTest.m Suppress output * Update GenerationTest.m Suppress warning that happens due to wrong values in the file's version attribute in the source schemas for v 2.2.0 and 2.6.0 * Update README.md Added section about setting up dynamically loaded filters * Update PynwbTutorialTest.m Move test tags to methods block, as testtags in classdef breaks the sphinx/docs build * Expand test README with dynamic filters and TestTags convention Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Smaller test improvements * Add resolution to spike times in Units table Add this so that tutorial is aligned with the nwbinspector checks (check_units_resolution_is_set) * Move coverage skip flag to workflow env * Update TutorialTest.m Improve test diagnostic message * Update run_tests.yml * Run Tests for PR t any branch * Refactor constructor generation (#786) * Refactor constructor validation and mixin setup logic - Moved mixin setup and DynamicTable validation logic from fillBody and fillCheck into the main constructor generation flow. - Introduced isDynamicTableDescendent helper for cleaner ancestry checks. - Removed the now-redundant fillCheck function. * Update fillConstructor.m Fix warning suppression in sprintf statement (add escape char) * regenerate types * fix spelling * Fix test Fixes bugs in test * Fix colnames in test * Update +file/fillConstructor.m Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * regenerate types --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update TutorialTest.m * Support DataPipe in verifyContainerEqual Extend the test helper to treat types.untyped.DataPipe like DataStub by calling load() on actualValue before comparing. * Rename SKIP_PYNWB var and update CI/tests Replace the old SKIP_PYNWB_COMPATIBILITY_TEST_FOR_TUTORIALS env var with SKIP_PYNWB_TESTS across tests and nwbtest.m, and update +tests/nwbtest.default.env. Update CI workflow matrix keys and usage in prepare_release.yml, run_tests.yml, and configurations/matlab_release_matrix_strategy.yml to use the new key (matrix.skip-pynwb-tests) and wire the MATLAB setenv accordingly. Move conditional installation of pynwb into the workflows (install pynwb only when skip-pynwb-tests == '0') and remove the direct git+ dependency from +tests/requirements.txt (update comment to reflect conditional installs). Also add TestTags = {'UsesPython'} to PyNWBIOTest. These changes centralize the CI control of pynwb installation and standardize the skip variable name. * debug readthedocs failed build checking if this class attribute trips up docs build * Update PyNWBIOTest.m Move TestTags to methods blocks. Check if that fixes issue with sphinx build of docs * Add subject section to domain tutorials * Update generated html- and m-files * Update electrode/imaging plane location to valid term in tutorials * Update inspectNwbFile.m * Update inspectNwbFile.m * Make subject ids/descriptions consistent * Decouple stable CI from pynwb/nwbinspector dev branches Install pynwb and nwbinspector from PyPI stable releases in the main test workflow so CI only fails due to matnwb regressions. Add a separate weekly workflow that tests against the dev branches to catch upstream incompatibilities early without blocking PRs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Update requirements.txt * Update nwbtest.default.env Added more details in comments Removed unused vars NWB_TEST_DEBUG, GITHUB_TOKEN Added PYNWB_REPO_DIR * Updated test workflows to check out pynwb in the runner * Refactor PynwbTutorialTest to use pre-cloned pynwb repo Replace custom venv/download/GitHub API infrastructure with a simple approach: read tutorial files from PYNWB_REPO_DIR env var pointing to a pre-cloned pynwb repo, and run them against the system Python. The CI workflows are now responsible for cloning the repo and setting the env var. TestTags = {'UsesPython'} added so the tag selector in the dev workflow picks up these tests, and SKIP_PYNWB_TESTS correctly excludes them on older MATLAB releases. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * change checkout location for pynwb * Add test suite README for new developers Covers prerequisites, running tests via nwbtest() and the MATLAB unit testing framework, Python dependency setup, environment variable configuration, and test authoring conventions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix pynwb repo checkout path in CI workflows actions/checkout requires path to be within the workspace directory. Use path: pynwb-repo (inside workspace) instead of ../pynwb-repo. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Update untypedSetTest.m Suppress output * Update GenerationTest.m Suppress warning that happens due to wrong values in the file's version attribute in the source schemas for v 2.2.0 and 2.6.0 * Update README.md Added section about setting up dynamically loaded filters * Update PynwbTutorialTest.m Move test tags to methods block, as testtags in classdef breaks the sphinx/docs build * Expand test README with dynamic filters and TestTags convention Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Smaller test improvements * Add resolution to spike times in Units table Add this so that tutorial is aligned with the nwbinspector checks (check_units_resolution_is_set) * Move coverage skip flag to workflow env * Update TutorialTest.m Improve test diagnostic message * Update run_tests.yml * Validate specialized DynamicTableRegion targets * Run Tests for PR t any branch * Refactor constructor generation (#786) * Refactor constructor validation and mixin setup logic - Moved mixin setup and DynamicTable validation logic from fillBody and fillCheck into the main constructor generation flow. - Introduced isDynamicTableDescendent helper for cleaner ancestry checks. - Removed the now-redundant fillCheck function. * Update fillConstructor.m Fix warning suppression in sprintf statement (add escape char) * regenerate types * fix spelling * Fix test Fixes bugs in test * Fix colnames in test * Update +file/fillConstructor.m Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * regenerate types --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Ben Dichter <ben.dichter@gmail.com>

ehennestad marked this pull request as draft January 27, 2026 20:50

ehennestad marked this pull request as ready for review January 27, 2026 21:20

ehennestad requested a review from Copilot January 27, 2026 21:20

Copilot started reviewing on behalf of ehennestad January 27, 2026 21:20 View session

Copilot AI reviewed Jan 27, 2026

View reviewed changes

Comment thread +file/fillConstructor.m Outdated

ehennestad requested a review from bendichter January 27, 2026 21:34

bendichter previously approved these changes Feb 24, 2026

View reviewed changes

bendichter enabled auto-merge (squash) February 24, 2026 15:03

ehennestad dismissed bendichter’s stale review via d61d32b March 10, 2026 12:50

ehennestad and others added 8 commits April 1, 2026 10:28

Update fillConstructor.m

59b0188

Fix warning suppression in sprintf statement (add escape char)

regenerate types

c42720b

fix spelling

bb99533

Fix test

192e09a

Fixes bugs in test

Fix colnames in test

86e4dda

Update +file/fillConstructor.m

bc2f847

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

regenerate types

b9da70e

ehennestad changed the base branch from main to decouple-ci-testing-from-pynwb-dev April 1, 2026 08:29

ehennestad force-pushed the refactor-constructor-generation branch from f5b761f to b9da70e Compare April 1, 2026 08:29

bendichter merged commit 39fdc28 into decouple-ci-testing-from-pynwb-dev Apr 1, 2026
11 of 12 checks passed

ehennestad mentioned this pull request Apr 17, 2026

[Bug]: Generation of table where there are colum names that don't correspond to any columns does not fail. #574

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor constructor generation#786

Refactor constructor generation#786
bendichter merged 8 commits intodecouple-ci-testing-from-pynwb-devfrom
refactor-constructor-generation

ehennestad commented Jan 27, 2026 •

edited

Loading

Uh oh!

codecov bot commented Jan 27, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

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

Uh oh!

Motivation

Changes

How to test the behavior?

Checklist

Uh oh!

codecov bot commented Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ehennestad commented Jan 27, 2026 •

edited

Loading

codecov bot commented Jan 27, 2026 •

edited

Loading