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

Optimize ArbitraryMotion #396

Closed
wants to merge 9 commits into from
Closed

Optimize ArbitraryMotion #396

wants to merge 9 commits into from

Conversation

pvillacorta
Copy link
Collaborator

This PR is intended to address #371 and #372

Copy link

codecov bot commented May 31, 2024

Codecov Report

Attention: Patch coverage is 51.35135% with 18 lines in your changes missing coverage. Please review.

Project coverage is 89.25%. Comparing base (27ae34f) to head (69035fd).
Report is 1 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #396      +/-   ##
==========================================
- Coverage   89.47%   89.25%   -0.23%     
==========================================
  Files          43       44       +1     
  Lines        2728     2727       -1     
==========================================
- Hits         2441     2434       -7     
- Misses        287      293       +6     
Flag Coverage Δ
base 85.68% <65.38%> (-0.74%) ⬇️
core 87.50% <18.18%> (+0.56%) ⬆️
files 93.70% <ø> (ø)
komamri 93.96% <ø> (ø)
plots 89.64% <ø> (ø)

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

Files Coverage Δ
KomaMRIBase/src/KomaMRIBase.jl 100.00% <ø> (ø)
KomaMRIBase/src/datatypes/Phantom.jl 84.86% <ø> (ø)
KomaMRICore/src/KomaMRICore.jl 100.00% <ø> (ø)
KomaMRIPlots/src/ui/DisplayFunctions.jl 90.47% <ø> (ø)
KomaMRICore/src/simulation/GPUFunctions.jl 62.85% <66.66%> (+9.91%) ⬆️
KomaMRICore/src/simulation/GPUArbitraryMotion.jl 0.00% <0.00%> (ø)
...se/src/datatypes/phantom/motion/ArbitraryMotion.jl 84.12% <65.38%> (-14.09%) ⬇️

@pvillacorta
Copy link
Collaborator Author

pvillacorta commented May 31, 2024

By the moment, I just got rid of the ux, uy, and uz fields of ArbitraryMotion struct, by:

  • Creating the interpolation functions "on the fly". Now they are not stored in the structure
  • What I think is the most significant change: pass from an array of 1D interpolations to a unique 2D interpolation function:
    What I am doing is a 2D (spins and time) Gridded Interpolation, with NoInterp in spin dimension and Gridded(Linear()) in time. See: https://juliamath.github.io/Interpolations.jl/v0.14/control/#Gridded-interpolation

With this changes, I executed the example below:

using KomaMRI

phantom = brain_phantom2D()

N_spins = length(phantom)
N_tdiscrete = 10 # Number of discrete times
duration = [1.0]

dx = dy = dz = 0.01.*rand(N_spins, N_tdiscrete)
phantom.motion = ArbitraryMotion(duration, dx, dy, dz) 

sys = Scanner()
seq = PulseDesigner.EPI_example() 

raw_signal = simulate(phantom, seq, sys)

In master brach:
8.516640 seconds (46.02 M allocations: 1.549 GiB, 5.05% gc time)

Now:
0.210349 seconds (111.28 k allocations: 152.714 MiB, 1.75% gc time)

Maybe that is enough improvement and we do not need to consider re-using the weights (in fact, I think this does it internally)

I should do more tests during these days

@pvillacorta
Copy link
Collaborator Author

I created a new branch optimize-arbitrary-parallel in which I initialize the interpolation functions and store them in the ExplicitArbitraryMotion struct. This does not mean pre calculating the trajectories, it just means initializing them, but the interpolations are still done "on the fly". This way, we avoid a lot of CPU->GPU transfers. The results are even better than before:
0.160241 seconds (86.02 k allocations: 2.652 MiB)

@pvillacorta
Copy link
Collaborator Author

pvillacorta commented Jun 14, 2024

A new branch, optimize-arbitrary-view, was created. This branch initializes the interpolating functions directly in GPU, so CPU-GPU transfers are not needed for this now. The performance is even better, although with a bit more allocations than optimize-arbitrary-parallel:

0.135876 seconds (95.69 k allocations: 2.986 MiB)

In addition, a custom adapt function for ArbitraryMotion is not needed now. Maybe this can solve problems in #405 and #406. My branch has not included the changes from these last two PRs from @rkierulf yet, so maybe I will need to change something once this is done.

In short, since optimize-arbitrary-view is by far the preferred option, I will close this PR and open a new one with this branch: #408

@cncastillo cncastillo deleted the optimize-arbitrary branch June 21, 2024 14:19
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.

Re-use weights in interpolation for ArbitraryMotion Simplify ArbitraryMotion struct
1 participant