feat: generalize directory discovery - #111
Open
kaitj wants to merge 6 commits into
Open
Conversation
Benchmark Results
|
Coverage Report
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
4 tasks
kaitj
marked this pull request as ready for review
August 20, 2026 18:14
kaitj
force-pushed
the
feat/generalize-ents/phase3
branch
from
August 27, 2026 20:28
261e8af to
5704f41
Compare
kaitj
force-pushed
the
feat/generalize-ents/phase3
branch
from
August 27, 2026 20:30
5704f41 to
0a5a1f2
Compare
Replace hardcoded constants with schema-derived values in preparation for
entity-generic indexing (Phase 3 of the generalize-entities effort).
- Remove `_BIDS_SUBJECT_DIR_PATTERN` regex — will be replaced by schema-
derived entity patterns from `get_entity_regex()` in subsequent commits
- Derive `_BIDS_JSON_SIDECAR_EXCEPTION_SUFFIXES` from the BIDS schema via
`get_json_data_suffixes()` instead of hardcoding `{"coordsystem"}` — now
picks up all text-only JSON data file suffixes (coordsystem, events,
electrodes, scans, etc.)
- Add `_get_dataset_type()` helper that reads `DatasetType` from
`dataset_description.json`, falls back to `"derivative"` inside nested
parent directories, and defaults to `"raw"`
Replace all subject-specific detection functions with entity-generic ones
that derive patterns from the BIDS schema at call time, rather than relying
on module-level constants frozen to the default schema.
Key changes:
- Remove `_BIDS_SUBJECT_DIR_PATTERN`, `_BIDS_ROOT_ENTITY_DIR_PATTERN`,
`_BIDS_JSON_SIDECAR_EXCEPTION_SUFFIXES` — no module-level constant should
call `load_bids_schema()` since it freezes values to the default schema
- Add `_compile_entity_dir_pattern()` to compile regex from entity prefixes
and format patterns
- Generalize `_is_bids_subject_dir` → `_isis_bids_entity_dir`, `_contains_bids_subject_dirs` → `_contains_bids_entity_dirs`,
`_find_bids_subject_dirs` → `_find_bids_entity_dirs`,
`_index_bids_subject_dir` → `_index_bids_entity_dir`
- Add `_resolve_entity_dirs()` orchestrator that tries primary root entity
types first and falls back to all known types
- Thread `BIDSSchemaAdapter` through the indexing pipeline so all detection
functions use the user-specified schema rather than a default
- `find_bids_datasets()` and `_is_bids_dataset()` accept an optional
`schema` parameter for schema-aware dataset discovery
- Generalize `_is_bids_json_sidecar()` to derive exception suffixes from
`get_json_data_suffixes()` instead of hardcoded `{"coordsystem"}`,
correctly recognizing ~300 additional JSON data files (channels,
motion, events, etc.) that were previously filtered out as sidecars
- Update all tests for new function signatures and revised row counts
- Replace hardcoded _BIDS_DATATYPE_PATTERN regex with _build_datatype_pattern(adapter), built at runtime from the schema's directory entities and format patterns (lru_cache'd) - _parse_bids_datatype now takes an adapter instead of using the module-level constant - parse_bids_entities accepts optional `schema` kwarg; the cached implementation _cache_parse_bids_entities now takes the adapter as a cache-key parameter so results are per-schema - clear_schema_caches() also clears _build_datatype_pattern and _cache_parse_bids_entities - Thread the adapter through _indexing.py call sites
- _load_bidsignore_patterns(root) reads root-level .bidsignore, skipping blank lines and # comments (lru_cache'd on root string) - _is_bidsignored(path, root) matches each pattern against both the full relative path and the bare filename, so patterns like sub-A01_*bold* work for files in datatype subdirectories - _index_bids_entity_dir skips ignored files via a combined `not _is_bids_file(...) or _is_bidsignored(...)` guard - test_batch_index_dataset count updated: 10052 → 9616 (5 bids-examples datasets ship a .bidsignore) - 7 new tests covering pattern parsing, comment/blank skipping, absent-file inclusion, and both matching modes
The five .bidsignore tests each rebuilt a near-identical minimal dataset inline. Move that setup into a shared `bidsignore_dataset` fixture in conftest.py (alongside the existing symlink_dataset fixture) and refactor the tests to use it. test_bidsignore_ignores_comments_and_blanks now asserts 2 files are indexed (up from 1): the shared fixture carries both a T1w and a bold file, so a comments/blanks-only .bidsignore provably excludes nothing.
A pass over to clean up comments and docstrings, with no behavior changes: - Add structured Args/Returns docstrings to the public get_arrow_schema and get_column_names (dropping the stale TODO in the latter). - Remove vague or code-restating inline comments and merge a few that spanned multiple lines. - Trim redundant free-floating prose from a few docstrings. - Remove group header comments in tests
kaitj
force-pushed
the
feat/generalize-ents/phase3
branch
from
September 2, 2026 17:38
0a5a1f2 to
b77d580
Compare
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
1 participant
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.
This PR:
Phase 3 of the generalized entities refactor, building on top of phase 2 (#109). A refactor that facilitates BIDS indexing for any root entity type (subject, template, cohort), rather than hardcoded
sub-*directories. Discovery/detection internals replaced with entity-generic ones, ands BIDS value they depended on are derived from the BIDS schema at call time instead of module-level constants._is_bids_subject_dir→_is_bids_entity_dir,_find_bids_subject_dirs→_find_bids_entity_dirs,_index_bids_subject_dir→_index_bids_entity_dir(all private); new_resolve_entity_dirs()orchestrator;_BIDS_SUBJECT_DIR_PATTERN/_BIDS_DATATYPE_PATTERNremoved._is_bids_file/_is_bids_json_sidecarmatch any entity prefix; JSON data-file suffixes come fromget_json_data_suffixes(), so ~300+ JSON data files (channels,events,motion, …) now index correctly..bidsignoresupport — root-level only, one glob per line (#comments/blanks skipped), matched against both the full relative path and the bare filename.index_dataset()gainsfilters(entity key → glob pattern(s)) and deprecatesinclude_subjects;find_bids_datasets()/batch_index_dataset()accept an optionalschema; new index columnsdataset_name,dataset_type,bids_version.--filter ENTITY=PATTERN(repeatable) and--schema;--subjectsdeprecated in favor of--filter sub=.....bidsignore, the new index columns, and the schema/entity helpers; sharedadapterandbidsignore_datasetfixtures.Behavioral note: the index now includes JSON data files previously misclassified as sidecars, so table row counts are larger than on
main.Type of Change
Related Issue(s)
Stack created with GitHub Stacks CLI • Give Feedback 💬