Skip to content

fix: remove VectorData unit generation patch#823

Draft
ehennestad wants to merge 6 commits into
mainfrom
codex/remove-vectordata-unit-patch
Draft

fix: remove VectorData unit generation patch#823
ehennestad wants to merge 6 commits into
mainfrom
codex/remove-vectordata-unit-patch

Conversation

@ehennestad
Copy link
Copy Markdown
Collaborator

@ehennestad ehennestad commented May 19, 2026

Motivation

NWB schema 2.10 introduces types.core.TimestampVectorData.unit (and types.core.DurationVectorData.unit) with fixed value seconds. MatNWB currently injects a hidden unit = "volts" property onto the types.hdmf_common.VectorData class as an older Units-table compatibility patch.

The current type generation pipeline now generates an invalid TimestampVectorData class definition under NWB schema 2.10.0, because a property (unit in this case) can not be defined on both a subclass (TimestampVectorData) and a superclass (VectorData).

This PR fixes that by removing the hidden units property from the VectorData class.

What changed

  • Removed units property compatibility patch when generating VectorData class
  • Removed the generated Units compatibility backfill that copied nested VectorData.unit values into promoted *_unit properties, e.g. obj.waveforms.unit into obj.waveforms_unit

Additional details

The Units-table compatibility patch creates unit, sampling_rate and resolution as hidden properties on the VectorData class in order to assign those properties to VectorData for the waveform_mean, waveform_sd, waveforms columns of a Units table.

This PR removes only the global VectorData.unit patch. Units-table waveform unit attributes are already handled by the schema-driven promoted-container API recently added in #797 (waveform_mean_unit, waveform_sd_unit, and waveforms_unit), so MatNWB no longer needs to leak unit onto all VectorData subclasses.

We could also remove the compatibility patch for sampling_rate and resolution in this PR, but that would risk breaking existing scripts/conversion code that depends on the legacy syntax. Removing only the unit patch is safer, because this was already a read-only property, whereas sampling_rate and resolution are also settable via the API.

This PR also removes the generated Units compatibility backfill that copied nested VectorData.unit values into promoted *_unit properties, e.g. obj.waveforms.unit into obj.waveforms_unit. That backfill was introduced in #797 to preserve the old nested syntax, but it depends on the same global VectorData.unit patch removed here. The promoted *_unit properties remain generated and exported directly from the containing Units object.

How to test the behavior?

runtests('tests.unit.file.VectorDataPatchTest')

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

@codecov
Copy link
Copy Markdown

codecov Bot commented May 19, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.41%. Comparing base (3e8407b) to head (1e2fbbc).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #823      +/-   ##
==========================================
- Coverage   95.42%   95.41%   -0.01%     
==========================================
  Files         215      215              
  Lines        7648     7632      -16     
==========================================
- Hits         7298     7282      -16     
  Misses        350      350              

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

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Removes MatNWB’s legacy, schema-injecting VectorData.unit compatibility patch to prevent property redefinition conflicts in NWB schema 2.10+ (e.g., TimestampVectorData.unit), while relying on the schema-driven promoted Units.*_unit API introduced in #797.

Changes:

  • Removed the hidden read-only unit="volts" injection from types.hdmf_common.VectorData (constructor parsing and export-conditional attribute write).
  • Removed generator-side VectorData.unit patching and related export/hidden-property special-casing.
  • Added a unit test asserting VectorData no longer has unit, while sampling_rate/resolution remain and Units exposes waveform_*_unit promoted properties.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
+types/+hdmf_common/VectorData.m Drops the generated hidden unit property and stops writing /unit on export.
+tests/+unit/+file/VectorDataPatchTest.m Adds coverage ensuring unit is not injected into VectorData and promoted Units unit properties exist.
+file/processClass.m Removes the generator patch that injected the unit attribute into VectorData.
+file/fillExport.m Removes VectorData-specific unit export conditional generation.
+file/fillClass.m Stops treating unit as a special hidden VectorData property during class generation.
+file/+internal/isPropertyHidden.m Stops hiding unit for VectorData in generator internals.

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

@ehennestad ehennestad enabled auto-merge May 19, 2026 16:44
@ehennestad ehennestad disabled auto-merge May 19, 2026 17:26
@ehennestad ehennestad marked this pull request as draft May 19, 2026 17:26
@ehennestad
Copy link
Copy Markdown
Collaborator Author

ehennestad commented May 19, 2026

@bendichter

Just realised that TimestampVectorData and DurationVectorData also defines the resolution attribute.
In my opinion, the simplest fix is to remove the add-hoc VectorData patch entirely, as it was already made redundant in PR #797

The drawback is that existing(?) code relying on setting the resolution of VectorData objects that go into the waveform* columns of a Units table will break, and it is not straight-forward to add a specific error message for such breakage.

The property was already hidden and the syntax was not described in any tutorials, so I expect the number of affected users would be very small.

Would appreciate your input!

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.

2 participants