PennLINC / qsirecon

Reconstruction of preprocessed q-space images (dMRI)
https://qsirecon.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
3 stars 1 forks source link

[CI] Speed up pyafq test #57

Closed mattcieslak closed 1 month ago

mattcieslak commented 1 month ago

This removes a couple bundles from the pyafq tests. hopefully this will speed up the test a bit

mattcieslak commented 1 month ago

@36000 would you mind taking a look at this PR? I'm not sure where I'm going wrong, but it looks like I can't get pyafq to run with more than 1 cpu. Here's the resource usage from the previous run. where it should have been using 8 cpus.

codecov-commenter commented 1 month ago

Codecov Report

Attention: Patch coverage is 16.66667% with 5 lines in your changes missing coverage. Please review.

Project coverage is 33.28%. Comparing base (9ab7939) to head (3b495c1). Report is 2 commits behind head on main.

Files Patch % Lines
qsirecon/interfaces/pyafq.py 25.00% 3 Missing :warning:
qsirecon/workflows/recon/pyafq.py 0.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #57 +/- ## ======================================= Coverage 33.27% 33.28% ======================================= Files 56 56 Lines 6779 6780 +1 Branches 888 887 -1 ======================================= + Hits 2256 2257 +1 Misses 4432 4432 Partials 91 91 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

36000 commented 1 month ago

I think it should be:

kwargs["segmentation_params"]["parallel_segmentation"]["n_jobs"] = self.inputs.n_procs

But I don't think this is even necessary. The part of the pipeline this parallelizes takes almost no time in these tests as the seed number is so low. I think most of the test time is taken to fit the mapping from subject to template space. I don't have any code right now to import the mapping from QSIprep for the current versions of QSIprep/pyAFQ, though I have tried.

I wonder if you should fit the mapping once beforehand and save it somewhere (like on figshare, or in the dockerfile) and just load it for the tests, instead of fitting it for the tests.

36000 commented 1 month ago

It would be both the prealign and mapping. These are what the filenames look like for the stanford hardi dataset: sub-01_ses-01_desc-prealign_from-DWI_to-MNI_xform.npy sub-01_ses-01_desc-mapping_from-DWI_to-MNI_xform.nii.gz

36000 commented 1 month ago

I can do this, but I won't be free to do this for about a month!

mattcieslak commented 1 month ago

oh, pyafq isn't using the mni transform from the preprocessed data? Is that what the previous itk_file code was doing?

mattcieslak commented 1 month ago

In the meantime is it possible to make the registration run in parallel?

36000 commented 1 month ago

That was what the previous ITK file code was doing. But I don't think the ITK code works anymore... something updated and changed its format. I spent some time but I couldn't figure out how to convert it anymore. Maybe nitransforms will solve this problem.

Anyways, since this is just a smoketest, we could also just use IdentityMap. It will just resample the ROIs from mni into subject space, which is wrong, but probably close enough for testing purposes. Especially for these few bundles

36000 commented 1 month ago

So: "mapping_definition": 'AffMap(affine_kwargs={"level_iters": [10, 10, 10]})', becomes: "mapping_definition": 'IdentityMap()',

mattcieslak commented 1 month ago

@36000 It didn't like that https://app.circleci.com/pipelines/github/PennLINC/qsirecon/134/workflows/cd025d5e-b362-4ca5-8eaf-cd378a841abc/jobs/1494

36000 commented 1 month ago

I see the problem. We will need to change pyAFQ. The PR is here: https://github.com/yeatmanlab/pyAFQ/pull/1153

36000 commented 1 month ago

how hard would it be to use that branch for the tests? To see if that fixes it?

mattcieslak commented 1 month ago

I'll probably just put this on pause until your PR makes it into PyAFQ. Could you point me to the area in PyAFQ where the itk map from qsiprep would be used? I may be able to get it working

36000 commented 1 month ago

This file holds the API for mapping, and FNIRTmap might be a good template for how to import mapping from another software: https://github.com/yeatmanlab/pyAFQ/blob/34c681750fa045f02946fced5640e1c1d76a5ec5/AFQ/definitions/mapping.py#L34