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.

Sign up for GitHub

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

Jump to bottom

[FIX] Remove task entity from DWI and PERF time series file templates #1703

Merged
merged 2 commits into from
Feb 13, 2024
Merged

[FIX] Remove task entity from DWI and PERF time series file templates #1703

merged 2 commits into from
Feb 13, 2024

Conversation

Remi-Gau
Copy link
Collaborator

@Remi-Gau Remi-Gau commented Feb 13, 2024 •
edited by tsalo
Loading

tsalo
tsalo approved these changes Feb 13, 2024
View reviewed changes
Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

The schema changes make sense to me. There's probably a way to define a shared base physio/stim rule that the MRI and MEG/EEG rules inherit from, but I don't know if it's worth it. I also looked over the filename templates for fMRI, DWI, ASL, and MEG, and they looked right as well.

effigies
effigies approved these changes Feb 13, 2024
View reviewed changes
Copy link
Collaborator

@effigies effigies left a comment

Choose a reason for hiding this comment

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

LGTM

@effigies effigies added the schema Issues related to the YAML schema representation of the specification. Patch version release. label Feb 13, 2024
@effigies effigies changed the title [FIX] Remove task entitiy from DWI and PERF time series [FIX] Remove task entity from DWI and PERF time series file templates Feb 13, 2024
@Remi-Gau
Copy link
Collaborator Author

There's probably a way to define a shared base physio/stim rule that the MRI and MEG/EEG rules inherit from, but I don't know if it's worth it.

From the quick check I did it would mean defining a "base" abstract rule that is not attached to any specific datatype and that the other "timeseries" would inherit from. At the moment I think this type of refactoring is a bit of an overkill.

@effigies effigies merged commit fea966c into bids-standard:master Feb 13, 2024
24 of 25 checks passed
@Remi-Gau Remi-Gau deleted the fix-1701 branch April 11, 2024 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@effigies effigies effigies approved these changes

@tsalo tsalo tsalo approved these changes

@erdalkaraca erdalkaraca Awaiting requested review from erdalkaraca erdalkaraca is a code owner

Assignees
No one assigned
Labels
schema Issues related to the YAML schema representation of the specification. Patch version release.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] timeseries files support task entity for ASL and DWI
3 participants
@Remi-Gau @effigies @tsalo