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

Replace niworkflows calls with dependency calls (second attempt) #299

Merged
merged 22 commits into from
Jun 1, 2023

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented May 31, 2023

Closes None, but works toward #199.

Changes proposed in this pull request

  • Create new aslprep.interfaces.niworkflows module.
    • Move aslprep.niworkflows.interfaces.registration.EstimateReferenceImage to this module.
  • Move aslprep.niworkflows.interfaces.registration._get_vols_to_discard() to aslprep.utils.misc.
  • Create new aslprep.utils.spaces module. This contains the bundled niworkflows version's spaces module.
  • Add to aslprep.workflows.asl.util module.
    • init_validate_asl_wf and init_asl_reference_wf are already present.
    • init_enhance_and_skullstrip_asl_wf:
      • There was an image dilation step applied to the input image, then a header-check to make sure the dilation step didn't mess up the header.
      • There was a pre_mask bool parameter for when the mask was vs. was not already calculated. The "mask was not pre-generated" logic has been removed, even though the parameter remains.
      • However, in the current version of sdcflows, init_enhance_and_skullstrip_bold_wf is only used for the PEPOLAR workflow, which shouldn't apply to ASL data. I think this means that we can switch to the newer version of sdcflows and not need the modified version of init_enhance_and_skullstrip_bold_wf we currently have.

Documentation that should be reviewed

@tsalo tsalo added the refactor Changes to the codebase that don't impact workflow inputs or outputs. label May 31, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jun 1, 2023

Codecov Report

Patch coverage: 64.48% and project coverage change: +15.37 🎉

Comparison is base (12112e9) 50.67% compared to head (9367df3) 66.05%.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #299       +/-   ##
===========================================
+ Coverage   50.67%   66.05%   +15.37%     
===========================================
  Files         139       74       -65     
  Lines       12657     6071     -6586     
===========================================
- Hits         6414     4010     -2404     
+ Misses       6243     2061     -4182     
Impacted Files Coverage Δ
aslprep/cli/run.py 0.00% <0.00%> (ø)
aslprep/sdcflows/interfaces/reportlets.py 0.00% <0.00%> (ø)
aslprep/sdcflows/viz/utils.py 0.00% <0.00%> (ø)
aslprep/sdcflows/workflows/fmap.py 0.00% <0.00%> (ø)
aslprep/sdcflows/workflows/gre.py 0.00% <0.00%> (-18.75%) ⬇️
aslprep/sdcflows/workflows/pepolar.py 0.00% <0.00%> (-18.69%) ⬇️
aslprep/sdcflows/workflows/phdiff.py 0.00% <0.00%> (-36.37%) ⬇️
aslprep/sdcflows/workflows/unwarp.py 0.00% <0.00%> (ø)
aslprep/utils/plotting.py 76.80% <ø> (-4.30%) ⬇️
aslprep/utils/sentry.py 0.00% <ø> (ø)
... and 23 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tsalo tsalo merged commit 56298bf into PennLINC:main Jun 1, 2023
@tsalo tsalo deleted the rm-niworkflows-2 branch June 1, 2023 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Changes to the codebase that don't impact workflow inputs or outputs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants