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

FIX: Correctly check SDCFlows registry of IntendedFor files #141

Merged
merged 7 commits into from
Dec 18, 2020

Conversation

oesteban
Copy link
Member

@oesteban oesteban commented Dec 11, 2020

Follows up on #140

EDIT 12/17/2020 --

Summary

This PR explores the new SDCFlows API to apply fieldmaps. Although the control over the available fieldmaps is almost none and many issues are still to be solved, this is a second step towards the integration.

Now, the CircleCI reports contain SDC results reportlets for ds001771 - one of them looks decent, the other needs some investigation.

This dataset contains two fake _fieldmap images - these are the two with the weird mask (the problem is that SDCFlows expects a magnitude image, but receives an EPI, because these are fake fieldmaps derived from the TOPUP solution).

@oesteban oesteban force-pushed the fix/checking-target-epi-names branch 3 times, most recently from 48626e1 to 675af9b Compare December 17, 2020 22:35
@oesteban oesteban requested a review from josephmje December 17, 2020 22:35
@oesteban
Copy link
Member Author

oesteban commented Dec 18, 2020

Failing for the same root cause of nipreps/sdcflows#158 (broken dependency of scikit-image).

@oesteban oesteban force-pushed the fix/checking-target-epi-names branch from 9b40390 to 5a08a86 Compare December 18, 2020 10:15
@josephmje
Copy link
Collaborator

This dataset contains two fake _fieldmap images - these are the two with the weird mask
(the problem is that SDCFlows expects a magnitude image, but receives an EPI, because these
are fake fieldmaps derived from the TOPUP solution).

Ha! I tested this out earlier and was wondering why the fieldmaps looked the same as the TOPUP solution.

@oesteban
Copy link
Member Author

Ha! I tested this out earlier and was wondering why the fieldmaps looked the same as the TOPUP solution.

Wonder no more. I think this is something that @edickie spotted a long while ago.

Copy link
Collaborator

@josephmje josephmje left a comment

Choose a reason for hiding this comment

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

Looks good to me! I just made a few small suggestions for the wording of the reportlet. Waiting for the tests to run to see what you mean by the pre-processing of the other fieldmap.

@oesteban oesteban force-pushed the fix/checking-target-epi-names branch from 7ba8492 to 6f27c1f Compare December 18, 2020 14:07
@oesteban oesteban merged commit 742a18f into nipreps:master Dec 18, 2020
@oesteban oesteban deleted the fix/checking-target-epi-names branch December 18, 2020 14:10
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