Skip to content

Add check_spatial_series_unit for SpatialSeries unit validation#685

Merged
h-mayorquin merged 10 commits intodevfrom
improve_unit_checking
Mar 26, 2026
Merged

Add check_spatial_series_unit for SpatialSeries unit validation#685
h-mayorquin merged 10 commits intodevfrom
improve_unit_checking

Conversation

@h-mayorquin
Copy link
Copy Markdown
Collaborator

@h-mayorquin h-mayorquin commented Mar 24, 2026

SpatialSeries has a free-form .unit field that defaults to "meters" in PyNWB but accepts any string. The NWB spec says it "should be SI unit" but nothing enforces this, so files in practice contain values like "m", "cm", "unknown", or empty strings. This check flags unrecognized unit strings to push the ecosystem toward consistent values.

The valid unit allowlist uses full SI words in plural form, matching the PyNWB convention: "meters", "centimeters", "millimeters", "micrometers", "degrees", "radians", and "pixels". I chose full words over abbreviations to be consistent with what PyNWB produces as defaults and what the existing check_compass_direction_unit already expects. SI length prefixes are included following the same principle as check_subject_weight, which accepts the full range of SI mass prefixes (kg, g, mg, etc.).

The check skips SpatialSeries objects inside a CompassDirection container since those are already covered by check_compass_direction_unit.

Related to #208 (general SI unit validation). This PR addresses one specific type rather than the broader question of enforcing SI conventions across all TimeSeries unit fields.

@h-mayorquin h-mayorquin self-assigned this Mar 24, 2026
@h-mayorquin h-mayorquin changed the title Add check that Units resolution is set Add check_spatial_series_unit for SpatialSeries unit validation Mar 24, 2026
@bendichter
Copy link
Copy Markdown
Contributor

This is tricky since sometimes the units are really unknown or arbitrary, but I suppose that's fine if it's just a violation and not a critical.

bendichter and others added 2 commits March 25, 2026 09:11
Add tests for unit=None and CompassDirection skip paths in
check_spatial_series_unit to achieve full patch coverage.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use 'kilometers' instead of 'degrees' so the test actually exercises the
CompassDirection ancestor check rather than passing due to a valid unit.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@bendichter bendichter self-requested a review March 25, 2026 13:14
@h-mayorquin
Copy link
Copy Markdown
Collaborator Author

This is tricky since sometimes the units are really unknown or arbitrary, but I suppose that's fine if it's just a violation and not a critical.

Maybe we can add a escape hatch of allowing them to intentionally write "n.a." in case that is the case, what do you think?

@h-mayorquin
Copy link
Copy Markdown
Collaborator Author

@bendichter
Copy link
Copy Markdown
Contributor

yeah I like the idea of allowing n.a.

@h-mayorquin h-mayorquin enabled auto-merge (squash) March 26, 2026 17:18
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.13%. Comparing base (061ee1e) to head (0860fde).
⚠️ Report is 1 commits behind head on dev.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #685      +/-   ##
==========================================
+ Coverage   79.92%   83.13%   +3.21%     
==========================================
  Files          48       48              
  Lines        1878     1886       +8     
==========================================
+ Hits         1501     1568      +67     
+ Misses        377      318      -59     
Files with missing lines Coverage Δ
src/nwbinspector/checks/__init__.py 100.00% <ø> (ø)
src/nwbinspector/checks/_behavior.py 100.00% <100.00%> (ø)

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@h-mayorquin h-mayorquin merged commit b9d55db into dev Mar 26, 2026
25 of 26 checks passed
@h-mayorquin h-mayorquin deleted the improve_unit_checking branch March 26, 2026 17:21
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.

3 participants