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

ENH: Serialize+parallelize 4D apply() into 3D+t and add 'low memory' loading #215

Merged
merged 24 commits into from
Aug 10, 2024

Conversation

oesteban
Copy link
Collaborator

Re-implements serialization (i.e., splitting a 4D transformation into 3D transformations). For time-dependent transforms (e.g., when also slice-time correcting), parallelization will require a different partition (spatial and/or temporal windowing).

@oesteban oesteban force-pushed the enh/reenable-parallelization-apply-214 branch from 0b2302f to b922fa5 Compare July 23, 2024 11:01
3D transform chains resulting of composing several transformations
(e.g., affine and deformation fields in spatial normalization) should
not be split into its components.

This is in contrast to lists of 3D transforms such as head-motion
correcting affines, where each applies to one timepoint.
These should be considered 4D and in some future they may integrate
slice timing correction in them.
@oesteban oesteban changed the title ENH: Serialize 4D apply() into 3D+t including parallelization ENH: Serialize 4D apply() into 3D+t Jul 31, 2024
@oesteban oesteban force-pushed the enh/reenable-parallelization-apply-214 branch from de81e23 to 8dd883d Compare July 31, 2024 07:19
@oesteban oesteban requested a review from effigies July 31, 2024 07:21
@oesteban
Copy link
Collaborator Author

oesteban commented Jul 31, 2024

Dropping parallelization for a future PR. Added through #220. This is ready for review.

Copy link

codecov bot commented Jul 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.76%. Comparing base (7c9eaed) to head (063e1f0).
Report is 45 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #215      +/-   ##
==========================================
+ Coverage   94.39%   94.76%   +0.36%     
==========================================
  Files          15       15              
  Lines        1713     1756      +43     
  Branches      323      328       +5     
==========================================
+ Hits         1617     1664      +47     
+ Misses         79       76       -3     
+ Partials       17       16       -1     
Flag Coverage Δ
unittests 94.76% <100.00%> (+0.36%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@oesteban oesteban force-pushed the enh/reenable-parallelization-apply-214 branch from ed1ffc8 to 026a10a Compare August 1, 2024 08:08
@oesteban oesteban force-pushed the enh/reenable-parallelization-apply-214 branch from a659355 to 7dcc78d Compare August 2, 2024 06:53
oesteban and others added 2 commits August 2, 2024 09:49
Resolves: #218.

Co-authored-by: Chris Markiewicz <effigies@gmail.com>
…214-dtypes

ENH: Implement a memory limitation mechanism in loading data
@oesteban oesteban changed the title ENH: Serialize 4D apply() into 3D+t ENH: Serialize+parallelize 4D apply() into 3D+t and add 'low memory' loading Aug 5, 2024
@oesteban
Copy link
Collaborator Author

oesteban commented Aug 5, 2024

@effigies I'm not planning on adding any more features within this PR, it is safe to start reviewing :)

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Overall the code looks like it should work. I think we're pushing a surprising amount of process control logic into this library, and would suggest seeing if it can be avoided. In particular, I don't think we should choose the number of threads, and I would try not to use multiprocessing.

The tests, I'm assuming, are mostly copied from their original locations? It's a lot to look over in detail. If there are new bits among large blocks of old, could you highlight them for review?

nitransforms/resampling.py Outdated Show resolved Hide resolved
nitransforms/resampling.py Outdated Show resolved Hide resolved
cval: float = 0.0,
prefilter: bool = True,
output_dtype: np.dtype = None,
serialize_nvols: int = SERIALIZE_VOLUME_WINDOW_WIDTH,
Copy link
Member

Choose a reason for hiding this comment

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

There's no parameter in the docstring, and the whole concept of serialization here is non-obvious. To the best of my understanding, the idea is that you want to parallelize over volumes but only if you hit a threshold that justifies the overhead of multiprocessing.

Have you compared the performance of multiprocessing versus async workers? If you're able to async or thread, you will have much less overhead and might not need this additional concept.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Overall it is a great interpretation.

There's something not so explicit: if you want real 4D interpolation (e.g., head motion + slice timing corrections) then you want to set serialize_nvols = np.inf because you can't split by volume (we've talked about doing this in a moving window, and then this parameter would map onto the width of that window).

Happy to document better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have added this parameter's description in the docstring, as it was missing. LMKWYT

@oesteban
Copy link
Collaborator Author

oesteban commented Aug 5, 2024

are mostly copied from their original locations?

Yes, and added parameters to exercise new branches.

It's a lot to look over in detail. If there are new bits among large blocks of old, could you highlight them for review?

Indeed, thanks for taking the time. I'll first see how to remove the process pool and then make a final pass to identify areas that may require focused attention.

Thanks a lot :)

@oesteban oesteban force-pushed the enh/reenable-parallelization-apply-214 branch from 436ae1d to b42b172 Compare August 6, 2024 07:02
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
@oesteban oesteban force-pushed the enh/reenable-parallelization-apply-214 branch from b42b172 to 063e1f0 Compare August 6, 2024 07:42
Comment on lines +233 to +236
# Number of data volumes
data_nvols = 1 if spatialimage.ndim < 4 else spatialimage.shape[-1]
# Number of transforms: transforms chains (e.g., affine + field, are a single transform)
xfm_nvols = 1 if transform.ndim < 4 else len(transform)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is new in this PR. Julien introduced the concept of transform.ndim to differentiate 3D transforms that require four parameters (e.g., displacements fields are typically stored as 4D NIfTIs, but are still 3D transforms) from 4D transformations (e.g., head-motion correction affines).

Following the logic of the previous version, the output is a 4D array if:

  • The input image is 4D and the transform is 3D (e.g., BOLD series and coregistration to T1w image), the same transform is applied to the series.
  • The input image is 3D and the transform is 4D (e.g., mapping a fieldmap onto all volumes of BOLD series), the same moving image is transformed through the transforms series.
  • The input image is 4D and the transform is 4D (e.g., head motion correction).

All these 4D transforms can be called "3D+t" if the coordinates in time are not moving (e.g., no slice-timing correction). If the transform is 3D+t, then we can serialize (i.e., apply one-by-one) and use an embarrassingly parallel approach for concurrence.

Julien also implemented 4D transforms -- this was new in his PR and would require more thorough testing (esp. testing moving coordinates in time). This code is executed if serialize_4d is not True.

@oesteban
Copy link
Collaborator Author

oesteban commented Aug 10, 2024

Seems to be working and the issues have been addressed - merging. Happy to get back to this in future post-merge reviews.

@oesteban oesteban merged commit b141a8e into master Aug 10, 2024
14 checks passed
@oesteban oesteban deleted the enh/reenable-parallelization-apply-214 branch August 10, 2024 07:51
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