Skip to content

Conversation

@yarikoptic
Copy link
Member

@yarikoptic yarikoptic requested a review from asmacdo May 23, 2025 15:00
Copy link
Member

@asmacdo asmacdo left a comment

Choose a reason for hiding this comment

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

LGTM. (my force push was to remove an additional commit I made in error)

@asmacdo
Copy link
Member

asmacdo commented May 23, 2025

CI failures are unrelated, but prevent running the script we changed. we should merge #140 first so we can let the CI be green prior to merge.

Also FWIW, its odd to me that raw is actually "bids converted". I'd have thought raw would refer to the original data, ie dicoms, but the doc is clear.

@yarikoptic
Copy link
Member Author

RE raw, it is what it is ATM. There was quite a bit of discussion, heh.

To be redefined, offer suggestion against BIDS specification. Here we just use best/recommended practice.

@asmacdo
Copy link
Member

asmacdo commented May 23, 2025

rebased to include shellcheck fix so we can see the tests run

@asmacdo
Copy link
Member

asmacdo commented May 23, 2025

Ah the real reason the test didnt run is fixed by #141, lets merge that first

@yarikoptic
Copy link
Member Author

Ok, green now - let's proceed

@yarikoptic yarikoptic merged commit 7373166 into master Jun 21, 2025
9 checks passed
@yarikoptic yarikoptic deleted the rf-sourcedata branch June 23, 2025 04:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants