Add DecompositionSeries unit and source_timeseries checks#687
Open
h-mayorquin wants to merge 10 commits into
Open
Add DecompositionSeries unit and source_timeseries checks#687h-mayorquin wants to merge 10 commits into
h-mayorquin wants to merge 10 commits into
Conversation
for more information, see https://pre-commit.ci
…k_for_decomposition_series' into implement_unit_check_for_decomposition_series
for more information, see https://pre-commit.ci
2 tasks
- Fix CHANGELOG PR reference (#676 -> #687). - Defer empty-unit ("") to the general check_missing_unit so the same field is not flagged by two BEST_PRACTICE_VIOLATION checks at once; the new check now owns only the PyNWB default "no unit" plus the metric-specific rules. Empty units are still reported via check_missing_unit. - Match the `metric` field case-insensitively so values like "Phase" are validated instead of silently skipped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #687 +/- ##
==========================================
+ Coverage 79.44% 83.13% +3.69%
==========================================
Files 48 49 +1
Lines 1897 1933 +36
==========================================
+ Hits 1507 1607 +100
+ Misses 390 326 -64
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DecompositionSeries stores the product of spectral analysis and has a
metricfield ("phase", "amplitude", "power") that determines what units are appropriate. PyNWB defaults the unit to"no unit", which is almost always wrong since the metric carries enough information to determine the correct unit.This PR adds two checks:
check_decomposition_series_unit(BEST_PRACTICE_VIOLATION) validates the unit field against the metric. When metric is "phase", the unit should be "radians" or "degrees". When metric is "amplitude" and source_timeseries is linked, the unit should match the source signal's unit (e.g. "volts" for an ElectricalSeries). The PyNWB default "no unit" and empty strings are flagged regardless of metric. The power case is skipped because there is no standard string format for squared units in NWB (see nwb-schema#446 for the ongoing discussion about adopting CMIXF-12 for unit notation).check_decomposition_series_source_timeseries(BEST_PRACTICE_SUGGESTION) flags when source_timeseries is not set. This field provides provenance about which signal was decomposed and enables the unit validation above. PyNWB itself already warns about this at construction time, but the inspector check catches it for files that were already written.Related to #208.