[WIP] refactor narrowing, add test, run narrow before copying - Githubissues

bids-apps / SPM

BIDS App containing an instance of the SPM software.

https://hub.docker.com/r/bids/spm/tags

Apache License 2.0

14 stars 12 forks source link

[WIP] refactor narrowing, add test, run narrow before copying #29

Open WillForan opened 10 months ago

WillForan commented 10 months ago

also address issue #27 and could supersede pull request #28

I pulled out themeta length check into it's own function and added a test. I also moved the narrow_participants call before temporary files are copied.

The test is only checking narrowing works, not any down stream effects. And I'm worried I've likely overlooked some important considerations. Please feel free to ruthlessly reject

Remi-Gau commented 10 months ago

will try to review in the coming days

makes me think that this kind of thing should probably be delegated to bids-matlab which is an update and better version of spm_bids

Remi-Gau commented 10 months ago

but already thanks for adding tests!!!

Remi-Gau commented 10 months ago

Note to self:

circle CI should run on PR
add code linting
use matlab github actions to run tests

Remi-Gau commented 10 months ago

Seems that we have a path issue when testing it:

https://app.circleci.com/pipelines/github/bids-apps/SPM/44/workflows/dd6d3670-b56f-419b-bb88-b8b6ec8f8021/jobs/220

Remi-Gau commented 10 months ago

Seems that we have a path issue when testing it:

https://app.circleci.com/pipelines/github/bids-apps/SPM/44/workflows/dd6d3670-b56f-419b-bb88-b8b6ec8f8021/jobs/220

that's because the new m file you have added is not copied into the container:

https://github.com/bids-apps/SPM/blob/68e5b93de860d841714ddb411d1675b65779c8cb/Dockerfile#L41