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: Address some reliability issues of the functional masking workflow #707

Closed
wants to merge 6 commits into from
Closed

FIX: Address some reliability issues of the functional masking workflow #707

wants to merge 6 commits into from

Conversation

oesteban
Copy link
Member

@pep8speaks
Copy link

pep8speaks commented Apr 15, 2022 •
edited
Loading

Hello @oesteban, Thank you for updating!

Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally, pip install flake8 and then run flake8 niworkflows.

Comment last updated at 2022-04-19 14:28:47 UTC

@oesteban oesteban mentioned this pull request Apr 15, 2022
Merged
mgxd
mgxd reviewed Apr 19, 2022
View reviewed changes
niworkflows/func/util.py Outdated Show resolved Hide resolved
mgxd
mgxd approved these changes Apr 19, 2022
View reviewed changes
Copy link
Contributor

@mgxd mgxd left a comment

Choose a reason for hiding this comment

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

I think this looks good (at least from locally testing ds210) - would you mind rebasing to maint/1.4.x since we'll need to backport this fix to fMRIPrep 21.0.x

@oesteban
Copy link
Member Author

I'm having a lot of conflicts - have we been merging all branches together to keep them in sync?

@oesteban oesteban force-pushed the fix/robustify-fmri-masking-workflow branch from 97be9e5 to 0509a28 Compare April 19, 2022 14:21
@oesteban oesteban changed the base branch from master to maint/1.4.x April 19, 2022 14:21
@oesteban
Copy link
Member Author

Okay, rebased. I'm going to generate a new branch masks/ so that the mask tests are triggered.

BTW - any ideas of why get data is failing on circle?

@effigies
Copy link
Member

Just from a quick look, it seems this violates our bug-fix rules. Does it make sense to target 1.5.x instead and just go in 22.0.0?

@effigies
Copy link
Member

effigies commented Apr 19, 2022 •
edited
Loading

BTW - any ideas of why get data is failing on circle?

datalad/datalad#6631

@oesteban
Copy link
Member Author

Just from a quick look, it seems this violates our bug-fix rules. Does it make sense to target 1.5.x instead and just go in 22.0.0?

Yes, I just found out that I propose to use a new interface. I'd be in favor of just going with this for 22.0.0

@oesteban oesteban force-pushed the fix/robustify-fmri-masking-workflow branch from 0509a28 to 2d4d964 Compare April 19, 2022 14:28
@oesteban oesteban mentioned this pull request Apr 19, 2022
Closed
@oesteban oesteban closed this Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgxd mgxd mgxd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@oesteban @pep8speaks @effigies @mgxd