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

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

WillForan
Copy link

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
Copy link
Contributor

Remi-Gau commented Nov 6, 2023

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
Copy link
Contributor

Remi-Gau commented Nov 6, 2023

but already thanks for adding tests!!!

@Remi-Gau
Copy link
Contributor

Remi-Gau commented Nov 6, 2023

Note to self:

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

narrow_participants.m Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think you could rewrite this test to be used directly by matlab testing framework?

Ideally with setup and teardown functions.

https://nl.mathworks.com/help/matlab/matlab_prog/write-test-using-setup-and-teardown-functions.html

If you don't have the bandwidth, let me know and I can give it a go

@Remi-Gau
Copy link
Contributor

Remi-Gau commented Nov 8, 2023

@Remi-Gau
Copy link
Contributor

Remi-Gau commented Nov 8, 2023

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:

SPM/Dockerfile

Line 41 in 68e5b93

COPY run.sh spm_BIDS_App.m pipeline_participant.m pipeline_group.m /opt/spm${SPM_VERSION}/

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.

2 participants