Skip to content

Warn instead of error when reading values invalid per the NWB schema#830

Draft
ehennestad wants to merge 10 commits into
add-validation-reporting-targetfrom
776-warn-on-reading-invalid-values
Draft

Warn instead of error when reading values invalid per the NWB schema#830
ehennestad wants to merge 10 commits into
add-validation-reporting-targetfrom
776-warn-on-reading-invalid-values

Conversation

@ehennestad

@ehennestad ehennestad commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

Fix #776

⚠️ Depends on #844 and #845.

Problem

MatNWB currently treats schema-invalid values as hard errors while reading NWB files. This is too strict: a single non-conforming value can make an otherwise usable file unreadable.

Solution

Update validators to use matnwb.common.validation.reportSchemaViolation. Schema violations remain errors during normal object construction and export, but become warnings during file reads so the original values remain accessible.

The following validators are updated in this PR:

  • Neurodata type validation: checkType
  • Soft-link target validation: validateSoftLink
  • Data type validation: checkDtype
  • Constrained data type validation: checkConstraint
  • DynamicTable consistency validation: checkConfig and getOutermostIndexColumnName

Constant-value validation (checkConstant) and shape validation (validateShape) are handled separately in #845.

Tests

nwbtest('Name', 'tests.unit.types.validators.CheckTypeTest')
nwbtest('Name', 'tests.unit.types.validators.ValidateSoftLinkTest')
nwbtest('Name', 'tests.unit.types.validators.CheckDtypeTest')
nwbtest('Name', 'tests.unit.types.validators.CheckConstraintTest')
nwbtest('Name', 'tests.unit.types.validators.DynamicTableCheckConfigTest')
nwbtest('Name', 'tests.unit.io.testCreateParsedType')

Checklist

  • Have you ensured the PR description clearly describes the problem and solutions?
  • Have you checked to ensure that there are no 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

This comment was marked as outdated.

@codecov

codecov Bot commented Jun 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.50746% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 95.30%. Comparing base (94e13bc) to head (5c1ae5a).

Files with missing lines Patch % Lines
+types/+util/checkConstant.m 94.73% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #830      +/-   ##
==========================================
- Coverage   95.31%   95.30%   -0.01%     
==========================================
  Files         218      220       +2     
  Lines        7634     7668      +34     
==========================================
+ Hits         7276     7308      +32     
- Misses        358      360       +2     

☔ View full report in Codecov by Harness.
📢 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 ehennestad force-pushed the 776-warn-on-reading-invalid-values branch from ae75126 to 5c1ae5a Compare June 19, 2026 13:25
@ehennestad ehennestad marked this pull request as draft June 20, 2026 06:20
@ehennestad ehennestad force-pushed the 776-warn-on-reading-invalid-values branch 2 times, most recently from b5a87a5 to 76ce128 Compare June 25, 2026 13:40
@bendichter

Copy link
Copy Markdown
Contributor

decision here is to push out a fix for the problems users are facing today, specifically validation of dataset shape and schema-locked value (e.g. "volts"). Then we can perhaps think about expanding to other types of validation in the future.

ehennestad and others added 9 commits July 1, 2026 13:53
* Add DynamicTableBase class for handwritten DynamicTable behavior

Introduce matnwb.neurodata.DynamicTableBase, an abstract handle base class that
owns the handwritten DynamicTable behavior the generated schema class cannot
express: row and column mutation, row retrieval, table conversion, clearing,
and consistency validation.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* Generate DynamicTable classes with a custom base class

fillClass attaches matnwb.neurodata.DynamicTableBase as a superclass for
DynamicTable via a customBaseClasses map, rather than injecting table methods
inline. The constructor and custom-constraint hooks now route the consistency
check through obj.ensureDynamicTableConsistency(). Remove the now-unused
fillDynamicTableMethods.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* Regenerate types against DynamicTableBase

DynamicTable now inherits matnwb.neurodata.DynamicTableBase and drops its inline
table methods; DynamicTable descendants call obj.ensureDynamicTableConsistency()
from their constructors and custom-constraint checks.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* Remove obsolete dynamictable add* wrappers

addColumn, addRow, and addTableColumn are superseded by DynamicTableBase methods
that call the addVararg* helpers directly. Document addVarargRow.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* Update DynamicTable tests for new addRow/addColumn signatures

Remove assertions that passing a MATLAB table to addRow/addColumn throws
NWB:DynamicTable; the new repeating-argument signatures reject that input via
argument validation instead.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* Route clear via the DynamicTable's clear method in dynamicTableTest

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
@ehennestad ehennestad force-pushed the 776-warn-on-reading-invalid-values branch from 1b9cbfe to 6d1fda0 Compare July 1, 2026 15:38
@ehennestad ehennestad changed the base branch from main to add-validation-reporting-target July 1, 2026 15:40
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.

[Feature]: Warning instead of error when reading values that are not valid according to the NWB schema

3 participants