Make Subject required by default in NWBFile::initialize()#320
Conversation
Co-authored-by: oruebel <10999845+oruebel@users.noreply.github.com>
- Add SubjectMetadata struct to NWBFile.hpp with optional fields for
species, sex, age, description, subjectId, genotype, strain, weight
- Add subject parameter to NWBFile::initialize() defaulting to an empty
SubjectMetadata{} (creates a Subject group by default); pass
std::nullopt to explicitly opt out
- Implement subject group creation in createFileStructure() with
neurodata_type/namespace attributes for NWB compliance
- Update testNWBFile.cpp to expect Subject in findOwnedTypes results
- Update workflow example to demonstrate Subject usage with actual data
- Remove --ignore=check_subject_exists from tests.yml and
upgrade_schema.yml CI workflows
Co-authored-by: oruebel <10999845+oruebel@users.noreply.github.com>
…thub.com/NeurodataWithoutBorders/aqnwb into copilot/make-subject-required-by-default
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #320 +/- ##
==========================================
- Coverage 84.69% 84.52% -0.18%
==========================================
Files 57 59 +2
Lines 2699 2805 +106
Branches 347 371 +24
==========================================
+ Hits 2286 2371 +85
- Misses 413 434 +21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds first-class support for the NWB /general/subject group and wires it into NWBFile::initialize() so CI nwbinspector validation no longer needs to ignore the subject-exists check, improving DANDI compliance.
Changes:
- Added a new
NWB::Subjectcontainer with aSubjectSpecfor initializing subject metadata. - Extended
NWBFile::initialize()to accept (optional) subject metadata and create/general/subjectaccordingly. - Updated tests, examples, and CI workflows to stop ignoring the subject-exists inspector check.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/testUtils.hpp | Adds a shared helper to generate consistent SubjectSpec test metadata. |
| tests/testSubject.cpp | New unit tests covering Subject registration, read/write of fields, and integration with NWBFile::initialize(). |
| tests/testRegisteredType.cpp | Adds Subject to the registered type path/name expectations. |
| tests/testRecordingWorkflow.cpp | Updates workflow test to initialize NWBFile with Subject metadata. |
| tests/testProcessingModule.cpp | Updates processing module tests to initialize NWBFile with Subject metadata. |
| tests/testNWBFile.cpp | Updates NWBFile tests for the new initialize signature and Subject default creation expectations. |
| tests/examples/testWorkflowExamples.cpp | Updates example workflow to initialize NWBFile with Subject metadata. |
| tests/examples/test_link_timeseries_example.cpp | Updates link-timeseries example to initialize NWBFile with Subject metadata. |
| tests/CMakeLists.txt | Adds the new testSubject.cpp to the test build. |
| src/nwb/NWBFile.hpp | Extends initialize() API to accept SubjectSpec (via std::optional). |
| src/nwb/NWBFile.cpp | Implements Subject creation during NWBFile initialization. |
| src/nwb/file/Subject.hpp | Introduces the Subject container and SubjectSpec definition and read accessors. |
| src/nwb/file/Subject.cpp | Implements Subject initialization/writing of provided metadata. |
| CMakeLists.txt | Adds Subject.cpp to the library build. |
| CHANGELOG.md | Documents addition of Subject and nwbinspector/initialize updates. |
| .github/workflows/upgrade_schema.yml | Removes ignore of subject-exists; keeps ignore for Allen CCF electrodes location check. |
| .github/workflows/tests.yml | Removes ignore of subject-exists; keeps ignore for Allen CCF electrodes location check. |
Suppressed comments (2)
src/nwb/NWBFile.hpp:113
- The PR title/issue says Subject should be created by default (with an explicit opt-out), but initialize() currently defaults subjectSpec to std::nullopt, so callers who omit the argument still get no /general/subject and will fail nwbinspector unless they remember to pass metadata.
const std::string& timestampsReferenceTime = "",
const std::optional<AQNWB::NWB::Subject::SubjectSpec>&
subjectSpec = std::nullopt);
src/nwb/file/Subject.hpp:78
- Leftover TODO suggests the initialize method is incomplete, but initialize() is already implemented in Subject.cpp. This looks like generated boilerplate that should be removed to avoid confusion.
// TODO: Update the initialize method as appropriate.
/**
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| /// @brief Weight at time of experiment. | ||
| std::optional<std::string> weight = std::nullopt; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Constructor for the Subject class must be made protected
Fixes #290
nwb-inspectorupgradedcheck_subject_existsto a warning/error for DANDI compliance. AqNWB did not yet supportSubjectand was silently ignoring this check in CI via--ignore=check_subject_exists.Changes
Subjectclass to represent the/general/subjectgroup in NWB files.Subject::SubjectSpecto configure a Subject and facilitate initialization also via NWBFile.--ignore=check_subject_existsoption to require subject.NWBFile::initializeto accept aSubjectSpecargument for subject metadata initialization