Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Bids Spec files #324

Merged
merged 2 commits into from
Aug 24, 2023
Merged

Implement Bids Spec files #324

merged 2 commits into from
Aug 24, 2023

Conversation

pvandyken
Copy link
Contributor

This is the first out of two PRs for the resolution of #245. Here, we add define the spec file and add a 0.0.0 file to represent the bids function as it's been until now. The api does not change at all here, but the bids function has been refactored to use the spec.

I also added an extensive test suite for the bids function.

The spec

The spec takes the following form (in yaml)

- entity: entityLongName
  tag: entity # optional
  dir: true # optional

The order of entries determines the order entities will be placed in by the bids function. datatype, suffix, extension, and prefix are considered nonstandard entities and must not be specified in the config. Unrecognized entities are, by default, placed at the end of the path before the suffix. However, at most one entry in the spec may be specified as:

- entity: "*"

This indicates a placeholder determining the insertion point of unrecognized entities.

Currently, no validation of the above specifications is enforced. The bids() function is hard-coded to use the 0.0.0 spec, which I already know is valid. In the future, when we support custom spec files, we will need to implement validation.

@github-actions github-actions bot requested review from akhanf, kaitj and tkkuehn August 12, 2023 17:26
@pvandyken pvandyken added the maintenance Updates or improvements that do not change functionality of the code label Aug 12, 2023
@pvandyken pvandyken marked this pull request as draft August 12, 2023 18:07
@pvandyken
Copy link
Contributor Author

In fact, this PR does introduce a slight behaviour change. Previously, if root was specified as ., it would be dropped from the resulting path. So:

bids(root=".", subject="001") == "sub-001/sub-001"

Now, the . will be kept as a path element:

bids(root=".", subject="001") == "./sub-001/sub-001"

Practically speaking, this shouldn't make much of a difference, as the paths are still canonically the same (It got me in some of my tests, but they are more strict about paths being exactly correct). But I think this change is more correct than previous behaviour.

@pvandyken pvandyken marked this pull request as ready for review August 14, 2023 17:33
Copy link
Contributor

@kaitj kaitj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At a glance, this seems largely good to me. A few minor comments and questions.

pyproject.toml Show resolved Hide resolved
snakebids/__init__.py Show resolved Hide resolved
snakebids/paths/resources/spec.0.0.0.yaml Outdated Show resolved Hide resolved
snakebids/paths/presets.py Show resolved Hide resolved
Copy link
Contributor

@tkkuehn tkkuehn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor comment and a question, but this looks good and I won't need to see it again.

snakebids/paths/presets.py Outdated Show resolved Hide resolved
snakebids/tests/strategies.py Show resolved Hide resolved
Copy link
Contributor

@kaitj kaitj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be good - up to you about the minor comment but pending fixing the doc build error, this lgtm.

@pvandyken pvandyken force-pushed the feat/bids/spec branch 2 times, most recently from 6a60932 to ef3570e Compare August 23, 2023 19:46
Add spec files and getters
Bids() is refactored to use the new bids specs, paving the way for
future customizable bids functions, plus speeds it up 2-3 fold. Also
adds a brand new test suite.
@pvandyken pvandyken merged commit 8d3519e into khanlab:main Aug 24, 2023
@pvandyken pvandyken deleted the feat/bids/spec branch December 1, 2023 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Updates or improvements that do not change functionality of the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants