feat: add numberOfSessions to AssetsSummary - #409
Conversation
Aggregator counts unique (subject, session) pairs parsed from BIDS-style sub-* and ses-* tokens in asset paths (filename first, then directory components when the session token is omitted from the filename). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #409 +/- ##
===========================================
+ Coverage 48.27% 97.36% +49.08%
===========================================
Files 19 19
Lines 2430 2463 +33
===========================================
+ Hits 1173 2398 +1225
+ Misses 1257 65 -1192
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
# Conflicts: # dandischema/metadata.py
| if not found.get("session") and part.startswith("ses-"): | ||
| found["session"] = part.split("ses-", 1)[1] | ||
| # Fallback: ses- tokens that appear only in directory components (e.g. | ||
| # `sub-X/ses-Y/foo_acq-Z_bold.nii.gz`) should still be counted. To form | ||
| # the (subject, session) pair we also accept a directory-only subject, | ||
| # but we do not add such a subject to stats["subjects"] — subject | ||
| # counting remains driven by the filename and wasAttributedTo, matching | ||
| # prior behavior. | ||
| if not found.get("session"): | ||
| dir_subject = found.get("subject") | ||
| dir_session: Optional[str] = None | ||
| for directory in asset_path.parts[:-1]: | ||
| for part in directory.split("_"): | ||
| if dir_subject is None and part.startswith("sub-"): | ||
| dir_subject = part.split("sub-", 1)[1] | ||
| if dir_session is None and part.startswith("ses-"): | ||
| dir_session = part.split("ses-", 1)[1] | ||
| if dir_subject is not None and dir_session is not None: | ||
| found.setdefault("subject", dir_subject) | ||
| found["session"] = dir_session | ||
| stats["sessions"] = stats.get("sessions", []) | ||
| if found.get("subject") and found.get("session"): | ||
| pair = (found["subject"], found["session"]) | ||
| if pair not in stats["sessions"]: | ||
| stats["sessions"].append(pair) |
There was a problem hiding this comment.
too aisloppy -- easier to HI compose the logic but first potentially even generalize 2 prior blocks into a 'for' loop, then if no session was found in filename, go through path's parts and split on ses- and be done if any matches.
There was a problem hiding this comment.
Done in cdb37ee — collapsed the sub-/sample-/ses- filename parsing into a single loop over an entities list, and simplified the session fallback to scanning asset_path.parts[:-1] for a ses-X directory and stopping at the first match. Dropped the directory-only subject fallback since BIDS files always carry sub- in the filename.
yarikoptic
left a comment
There was a problem hiding this comment.
THANK YOU for the PR. Needs a little a human touch ;)
Per review feedback: generalize the per-prefix blocks (sub-, sample-, ses-) into a single loop over an entity list, and simplify the session fallback to a direct scan of path parts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Is the |
|
yes, I think patch makes sense for this while major is 0 |
candleindark
left a comment
There was a problem hiding this comment.
LGTM as the targeted datasets are valid BIDS datasets. (See other comments for more details).
| numberOfCells: Optional[int] = Field(None, json_schema_extra={"readOnly": True}) | ||
| numberOfSessions: Optional[int] = Field( | ||
| None, json_schema_extra={"readOnly": True} | ||
| ) # BIDS ses-* tokens, counted as unique (subject, session) pairs |
There was a problem hiding this comment.
Possibly add to the comment that this count assumes the dataset is valid BIDS dataset. (For example, a session without subject will not be counted)
There was a problem hiding this comment.
well, that's what I wonder -- since we do have sub-..._ses-... even in DANDI names, and we do count on those -- we better count sessions as well in non-BIDS names as well. but "AI logic" above is wrong for that... will comment there
There was a problem hiding this comment.
actually my initial read was wrong -- there is a generic handling in the filename at https://github.com/dandi/dandi-schema/pull/409/changes#diff-196e8d7f9869422a0dba629b1f58a9972a300d67308aec783b2847cdb50ef5ffR541
but wouldn't it work for DANDI names (no folder name) as well and thus just a matter of
| ) # BIDS ses-* tokens, counted as unique (subject, session) pairs | |
| ) # BIDS or DANDI ses-* tokens (in file name or path), counted as unique (subject, session) pairs |
?
Coverage's `source = dandischema` resolves to the in-repo tree, but the package was installed (non-editable) into the tox venv, so the source modules actually executed from site-packages and fell outside the measured tree. As a result Codecov recorded 0% for every source module (metadata.py, models.py, ...) — only the test files, which run from the repo, were counted. This dragged patch coverage below the project threshold even for fully tested changes. Setting `usedevelop = true` makes the measured source and the executed source the same in-repo files, so metadata.py now reports ~90% instead of 0% and Codecov can map diff lines correctly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Schema 0.7.0 is already published in dandi/schema, so adding the numberOfSessions field to AssetsSummary requires a new schema version (the release workflow guards against modifying published schemata). Also add a schema description for the field documenting the (subject, session) pair counting, mark the new test as ai_generated, and cover the case where ses- appears without any sub- token. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Summary
numberOfSessions: intfield toAssetsSummary, with a schema description documenting the counting semanticsDANDI_SCHEMA_VERSIONto 0.7.1, since 0.7.0 is already published in dandi/schema and the release workflow rejects modifications to published schemata(subject, session)pairs parsed from BIDS-stylesub-*/ses-*tokens in asset paths. Keying on the pair avoids the undercount when two subjects share a session id likeses-1sub-/sample-handling); directory components are scanned as a fallback soses-*tokens that appear only in the path (e.g.sub-X/ses-Y/foo_acq-Z_bold.nii.gz) are still countedCaveats
sub-/ses-tokens won't contribute. Note thatdandi organizeproducessub-X/sub-X_ses-Y.nwblayouts, so organized NWB dandisets with session ids are counted; only datasets that follow neither BIDS nor the organized layout are missed.Test plan
test_aggregate_number_of_sessionscovers: same subject across sessions, two subjects sharing a session id,ses-only in directory,ses-without anysub-, and the no-ses-casetest_aggregatefixtures updated with expectednumberOfSessionsvaluestest_aggregation_bidsnow assertsnumberOfSessions == 2268 passed, 17 skipped;tox -e lint,typingclean🤖 Generated with Claude Code