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

ENH: Add models for ds003, ds114 #160

Closed
wants to merge 7 commits into from

Conversation

effigies
Copy link
Contributor

Replaces #130.

* Initial capitals in JSON keys
* snake_case for variables
* AutoContrasts for dataset-level analysis
* High-pass filter cutoff
* Initial capitals in JSON keys
* snake_case for variables
* AutoContrasts for dataset-level analysis
* High-pass filter cutoff

Additionally, moved Split transformation to subject level, eliminating session-level step
@sappelhoff
Copy link
Member

Thanks, you seem to have made this PR from your old fork ... are you sure that this will not reintroduce some bad history?

Why not delete your old fork, make a fresh one, and proceed?

@effigies
Copy link
Contributor Author

I rebased on the current master.

@sappelhoff
Copy link
Member

I see ... I also just realized that we won't introduce bad history via PRs, but rather via direct pushes to master.

Is this ready to be merged? If you need a review, you'd have to tag a member that has some experience with the models spec (I am not familiar enough with that).

@effigies
Copy link
Contributor Author

No, it's possible for someone to introduce bad history, but it should show up as a ton of commits in the PR. Typically just asking them to rebase on master will do the job. But re-forking is fine, too.

I'll request review from @tyarkoni and @adelavega.

@tyarkoni
Copy link

tyarkoni commented May 18, 2019

I looked through the models (admittedly not very carefully, but short of trying to run them through pybids and fitlins, I don't know that my careful review is terribly helpful) and they LGTM.

Edit: one caveat is that the first model is missing intermediate steps between run and dataset, which I believe means that the dataset level should expect to receive multiple runs per session and/or subject. My recollection is that currently aggregating within these intermediate levels must be explicit (e.g., via autocontrast). Not sure if this is the intended behavior, and maybe my memory's wrong, but flagging it just in case.

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

to get this PR merged, a .SKIP_VALIDATION file (like in the derivatives ds) should be added to the ds003 and ds114 datasets, ... or the models directories added in this PR should be .bidsignored

@rwblair
Copy link
Member

rwblair commented Sep 21, 2023

Should live in bids stats model zoo

@rwblair rwblair closed this Sep 21, 2023
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.

5 participants