New Motion

[pvillacorta]

This PR is intended to address #410

Still to be done:

• Revise and change docstrings
• Improve tests
• Benchmarking (simulation times, allocations, accuracy...)
• Test with flow phantoms
• Remake phantoms from /examples/2.phantoms:
  • brain_motion
  • contracting_ring
  • flow phantom
• Define the Nspins parameter for all motion types (I don't know if for this PR) -> EDIT: Now called SpinRange (solves #376)

[rkierulf]

I think I've found the cause of the Simple and Arbitrary Motion test failures for Metal: in UnitTime.jl, this line is processed incorrectly:

return min.(max.((t .- t_start) ./ (t_end - t_start), zero(T)), oneunit(T))

It seems as if Metal has issues compiling the min and max broadcast together. However, if they are separated then it works as expected:

tmp = max.((t .- t_start) ./ (t_end - t_start), zero(T))
return min.(tmp, oneunit(T))

Making this change fixed the failing tests on my computer. I've also written JuliaGPU/Metal.jl#389 since this appears to be a Metal issue with Julia 1.10.

[codecov]

Codecov Report

Attention: Patch coverage is 80.33827% with 93 lines in your changes missing coverage. Please review.

Project coverage is 89.86%. Comparing base (4862b40) to head (e416800).
Report is 101 commits behind head on master.

Files with missing lines	Patch %	Lines
KomaMRIPlots/src/ui/DisplayFunctions.jl	75.90%	20 Missing ⚠️
KomaMRIBase/src/motion/motionlist/SpinSpan.jl	26.08%	17 Missing ⚠️
KomaMRIBase/src/motion/nomotion/NoMotion.jl	23.80%	16 Missing ⚠️
KomaMRIFiles/src/Phantom/Phantom.jl	85.29%	10 Missing ⚠️
KomaMRIBase/src/motion/motionlist/MotionList.jl	88.23%	8 Missing ⚠️
KomaMRIBase/src/datatypes/Phantom.jl	76.47%	4 Missing ⚠️
KomaMRIBase/src/motion/motionlist/TimeSpan.jl	78.94%	4 Missing ⚠️
...e/src/motion/motionlist/actions/ArbitraryAction.jl	89.47%	4 Missing ⚠️
KomaMRIBase/src/motion/motionlist/Motion.jl	85.00%	3 Missing ⚠️
.../motion/motionlist/actions/simpleactions/Rotate.jl	86.36%	3 Missing ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #442      +/-   ##
==========================================
- Coverage   90.93%   89.86%   -1.07%     
==========================================
  Files          53       54       +1     
  Lines        2934     3050     +116     
==========================================
+ Hits         2668     2741      +73     
- Misses        266      309      +43     

Flag	Coverage Δ
base	86.24% <76.95%> (-1.96%)	⬇️
core	93.07% <98.11%> (+0.45%)	⬆️
files	91.69% <85.29%> (-1.86%)	⬇️
komamri	93.98% <ø> (ø)
plots	88.43% <75.90%> (-0.88%)	⬇️

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

Files with missing lines	Coverage Δ
KomaMRIBase/src/KomaMRIBase.jl	100.00% <ø> (ø)
KomaMRIBase/src/motion/motionlist/Action.jl	100.00% <100.00%> (ø)
...Base/src/motion/motionlist/actions/SimpleAction.jl	100.00% <100.00%> (ø)
...tion/motionlist/actions/simpleactions/HeartBeat.jl	100.00% <100.00%> (ø)
KomaMRICore/ext/KomaoneAPIExt.jl	92.59% <ø> (ø)
KomaMRICore/src/KomaMRICore.jl	100.00% <ø> (ø)
KomaMRICore/src/simulation/Flow.jl	100.00% <100.00%> (ø)
KomaMRICore/src/simulation/Functors.jl	91.30% <100.00%> (+5.59%)	⬆️
...RICore/src/simulation/SimMethods/Bloch/BlochCPU.jl	100.00% <100.00%> (ø)
...RICore/src/simulation/SimMethods/Bloch/BlochGPU.jl	100.00% <100.00%> (ø)
... and 15 more

... and 4 files with indirect coverage changes

[github-actions]

KomaMRI Benchmarks

Benchmark suite	Current: e416800	Previous: a26d598	Ratio
MRI Lab/Bloch/CPU/2 thread(s)	226540521 ns	224991845.5 ns	1.01
MRI Lab/Bloch/CPU/4 thread(s)	137009550 ns	174863887 ns	0.78
MRI Lab/Bloch/CPU/8 thread(s)	143972736.5 ns	90572834 ns	1.59
MRI Lab/Bloch/CPU/1 thread(s)	414395326 ns	347400444 ns	1.19
MRI Lab/Bloch/GPU/CUDA	56269762.5 ns	57092571.5 ns	0.99
MRI Lab/Bloch/GPU/oneAPI	500507813 ns	522866366 ns	0.96
MRI Lab/Bloch/GPU/Metal	551246770.5 ns	568303458 ns	0.97
MRI Lab/Bloch/GPU/AMDGPU	34821151 ns	36981171 ns	0.94
Slice Selection 3D/Bloch/CPU/2 thread(s)	1019622442 ns	1151416902 ns	0.89
Slice Selection 3D/Bloch/CPU/4 thread(s)	624166270 ns	580233533 ns	1.08
Slice Selection 3D/Bloch/CPU/8 thread(s)	390068757.5 ns	341164837 ns	1.14
Slice Selection 3D/Bloch/CPU/1 thread(s)	2262459750.5 ns	1930414196.5 ns	1.17
Slice Selection 3D/Bloch/GPU/CUDA	100820723.5 ns	101438582.5 ns	0.99
Slice Selection 3D/Bloch/GPU/oneAPI	640338956 ns	636188156.5 ns	1.01
Slice Selection 3D/Bloch/GPU/Metal	554214166.5 ns	565478250 ns	0.98
Slice Selection 3D/Bloch/GPU/AMDGPU	59085335 ns	60929383 ns	0.97

This comment was automatically generated by workflow using github-action-benchmark.

[cncastillo]

You can preview the docs here (when they run):
https://juliahealth.org/KomaMRI.jl/previews/PR442

[cncastillo]

I left some minor comments, most of them are renaming stuff. The documentation part can be a different PR, but we may want to create an issue so we don't forget. At least let's make the CI happy.

Also, I'm not sure if writing/reading phantoms work. I assume it does as it is passing the tests.

I am not sure yet about the best way to specific to which part of the object the motion applies, it would be better for another PR.

Did you try Bloch and BlochSimple?

We may want to add a benchmark that includes motion.

[cncastillo]

Btw, the min(max) problem was already fixed in GPUCompiler.jl (JuliaGPU/GPUCompiler.jl#602), thanks to Ryan's report. So this is no longer necessary:

tmp = max.((t .- t_start) ./ (t_end - t_start), zero(T))
return min.(tmp, oneunit(T))

[pvillacorta]

Btw, the min(max) problem was already fixed in GPUCompiler.jl (JuliaGPU/GPUCompiler.jl#602), thanks to Ryan's report. So this is no longer necessary:

tmp = max.((t .- t_start) ./ (t_end - t_start), zero(T))
return min.(tmp, oneunit(T))

How can I apply those changes? Tests are still failing if I don't define the tmp variable

[cncastillo]

Nothing, the latest version of all the packages will be used. If the latest version of GPUCompiler is not being used (v0.27.0) that means there are some compatibility issues, and the Project.toml of KomaMRICore needs to be updated.

In the CI there are some errors due to renaming

LoadError: UndefVarError: `AbstractMotionList` not defined

[cncastillo]

Metal and oneAPI changed their compact to Julia 1.10 (next LTS), so we may want to do the same. If the problems are in Julia 1.9 that could be the reason.

Edit: metal has not registered a new version compatible with GPUcompiler 0.27 yet, so you can leave that part as it was for now.

[pvillacorta]

oneAPI is now the one that fails. I should revise it

[cncastillo]

Also @pvillacorta, I just remembered, but there needs to be a pretty important change in how the Phantoms are plotted. Please dispatch based on Phantom{Motion} or something similar to change plot_phantom_map, as we need to use plot_koma instead of Plot, as the last one breaks compatibility with VSCode and Pluto. This is more important than all the other comments.

[pvillacorta]

Also @pvillacorta, I just remembered, but there needs to be a pretty important change in how the Phantoms are plotted. Please dispatch based on Phantom{Motion} or something similar to change plot_phantom_map, as we need to use plot_koma instead of Plot, as the last one breaks compatibility with VSCode and Pluto. This is more important than all the other comments.

@cncastillo Okay, I will take this into account. I remember when I tried to solve this problem with the Plot and plot_koma: the main problem was the time slider, which somehow did not work with the plot function (the one called by koma_plot), and only worked with Plot. So there is two options for now:

1. Remove the slider. The plot will still be able to move (in fact, I am not 100% sure about this), but it would not be possible to control the time.
2. Dispatch. Use plot_koma for phantoms with NoMotion, and Plot for dynamic phantoms.

In terms of code stability, option 1 is better, with the disadvantage that we would lose the slider...
Tell me what you think

[cncastillo]

I prefer to have a separate method for plot_phantom_map if Phantom{Motion} has motion, so we don't loose the slider.

There will be code repetition, unless we refactor the functions, or make the slider work. For now this is the easiest way, we can clean it up later.

[cncastillo]

CUDA seems to be segfaulting on BuildKite, but I don't think that is our fault, so I re-run that job to see if it passes, also for Github's CI (Julia 1 / Ubuntu).

[cncastillo]

@pvillacorta The CI is saying this

┌ KomaMRICore
│  WARNING: method definition for #outflow_spin_reset!#54 at /var/lib/buildkite-agent/builds/gpuci-15/julialang/komamri-dot-jl/KomaMRICore/src/simulation/Flow.jl:18 declares type variable T but does not use it.
└  

Also, the CodeCov is failing because there's no test for the new plot_phantom_map with motion.

[pvillacorta]

Yep, I removed it in the commits where I tried to make spin_reset a Matrix{Bool}, but I am not pushing them and I forgot to change that.

Everything passes now, except one random test for oneAPI in buildkite. It is due to a @view call inside get_spin_coords, but it never happened before and there is nothing I have changed in the latest commits that could make it fail now. @cncastillo can you re-run that test?

[cncastillo]

It looks like a proper error, but I will re-run it.

EDIT: It may have been a fluke, as I realized that we are running Julia 1.10 twice now, as 1.11 is not out yet.

[cncastillo]

I am a little worried about this @pvillacorta

Benchmark suite	Current	Previous	Ratio
MRI Lab/Bloch/CPU/8 thread(s)	143972736.5 ns	90572834 ns	1.59

There seems to be a performance regression. I will merge it anyway, but if this is still the case (after the benchmarks are performed in master), I would need to ask you to investigate.