Add MeaningsTable

[rly]

Motivation

Requires #1374

Fix #1375

See hdmf-dev/hdmf-common-schema#91 for schema changes

How to test the behavior?

pytest

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?

[codecov]

Codecov Report

❌ Patch coverage is 96.33028% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.53%. Comparing base (e24a19c) to head (e259b2a).

Files with missing lines	Patch %	Lines
src/hdmf/common/table.py	96.10%	2 Missing and 1 partial ⚠️
src/hdmf/container.py	88.88%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1376      +/-   ##
==========================================
+ Coverage   92.45%   92.53%   +0.08%     
==========================================
  Files          42       42              
  Lines        9765     9859      +94     
  Branches     1983     1998      +15     
==========================================
+ Hits         9028     9123      +95     
+ Misses        462      461       -1     
  Partials      275      275              

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

[oruebel]

Overall, the code looks good. I added a few suggestions/comments.

[oruebel]

I added a few minor additional suggestion. Feel free to accept/reject as you see fit and ping me again

[rly]

Question: Feedback on BaseDynamicTable / DynamicTable Schema Refactoring

We're considering refactoring the DynamicTable schema hierarchy to support MeaningsTable. I'd like feedback on whether this approach is the right design choice.

Proposed Implementation

Schema (hdmf-common-schema/common/table.yaml):

BaseDynamicTable:
  data_type_inc: Container
  # Core table functionality: id, colnames, description, VectorData columns

DynamicTable:
  data_type_inc: BaseDynamicTable
  groups:
    - name: meanings_tables (quantity: '?')
      groups:
        - data_type_inc: MeaningsTable (quantity: '*')

MeaningsTable:
  data_type_inc: BaseDynamicTable  # NOT DynamicTable
  datasets: [value, meaning]
  links: [target -> VectorData]

Python (hdmf/common/table.py):

• BaseDynamicTable: Core table functionality (add_row, add_column, DataFrame conversion, etc.)
• DynamicTable: Adds meanings_tables field and related methods
• MeaningsTable: Extends BaseDynamicTable directly

Rationale

MeaningsTable should not have its own MeaningsTable (a meanings table for a meanings table is nonsensical), so MeaningsTable extends BaseDynamicTable to avoid inheriting the meanings_tables group.

Pros

1. Clean semantics: MeaningsTable cannot have nested MeaningsTable objects — enforced at schema level, providing type safety and clear intent

Cons

1. Design/codebase changes:
   • Code checking isinstance(obj, DynamicTable) won't match MeaningsTable
   • DynamicTableRegion points to BaseDynamicTable instead of DynamicTable
   • API code and documentation across all table-related types (e.g., VectorData, DynamicTableRegion, AlignedDynamicTable) now references BaseDynamicTable instead of DynamicTable
2. Increased complexity: Three schema types instead of two; users must understand when to use each

Since we expect most users to use DynamicTable, documentation and tutorials will continue to demonstrate use of DynamicTable instead of BaseDynamicTable.

Alternative Approach

Keep a single DynamicTable type and:

• Make meanings_tables group optional (already is with quantity: '?')
• Add validation/restriction in each API that MeaningsTable cannot contain meanings_tables
• Simpler hierarchy, same end result

Question

Is the structural enforcement (separate schema types) worth the added complexity, or would validation/restriction in each API on a single DynamicTable type be preferable?

[oruebel]

We're considering refactoring the DynamicTable schema hierarchy to support MeaningsTable.

Thanks for the helpful summary.

Pros

Here I would add:

• Enables extensions to use a table as is (without Meanings) rather than always having MeaningsTable in there. On the flip-side this is a Con of the alternative approach, which modifies all existing schema to add MeaningsTable without having the ability to remove it.

Cons

Here, I think the main con is the effort and potential unexpected larger downstream code-changes this may require.

Alternative Approach

• Add validation/restriction in each API that MeaningsTable cannot contain meanings_tables

Here, I would argue that we should not artificially restrict this in the API if the schema allows it. A check in the nwb-inspector would make sense or maybe even a warning in the API, but if the schema allows it, then adding this as custom logic seems unwieldy. Theoretically, defining meanings for columns of a meanings table is I think a valid thing to do, in particular since users can add custom columns in a MeaningsTable (which could have information that requires further definition), however, I agree that in most cases this is probably not the right way to do it.

I.e., I would argue that we either use:

1. use the proposed approach of adding BaseDynamicTable as a new base class, or
2. keep the single DynamicTable class (with options for MeaningsTables), which means allowing users to define meanings for columns of MeaningsTable (with maybe a best practice check in the nwb-inspector)

Personally, I think option 1 is cleaner, but will require significant effort such that the cost for implementing this may out-way the benefit. I think option 2 should be fine, but may require changes down-the-line if/when a real use-case for a BaseDynamicTable should arise.

Long story short, I think either option should meet the current needs reasonably well, and I think it is mainly a question of spending effort now to prepare for potential use cases vs. waiting for real use cases and spending the effort then. I am personally fine with either approach right now, but the more I think about, I'm starting to lean towards Option 2 (even-though I originally proposed Option 1 😉 )

[h-mayorquin]

I vote for option 2. I think that option 1 is a large modification just to exclude what seems like an edge case.

Also, is there similar art on the data frame data model that we can use as a compass?