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

Fix sum of Grads #486

Merged
merged 3 commits into from
Sep 26, 2024
Merged

Fix sum of Grads #486

merged 3 commits into from
Sep 26, 2024

Conversation

pvillacorta
Copy link
Collaborator

@pvillacorta pvillacorta commented Sep 26, 2024

This is a temporary change to solve a bug when rotating sequences:

julia> using KomaMRI
julia> seq = PulseDesigner.EPI_example()
julia> plot_seq(seq[1:10])

image

julia> seq_r = rotz/2) * seq
julia> plot_seq(seq_r[1:10])

image

This bug happens when rotating a Sequence, as we are implicitly calling +(Grad(0, 0), Grad(A, T)), and this function assigns the T, delay, rise and fall of the first Grad to the result. A temporary solution is to assign the maximum of each one of these values:

+(x::Grad, y::Grad) = Grad(x.A .+ y.A, max(x.T, y.T), max(x.rise, y.rise), max(x.fall, y.fall), max(x.delay, y.delay)) 

But a better solution would be to "stack" the gradients if they have different time-related parameters. See #487

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

KomaMRI Benchmarks

Benchmark suite Current: 13c690c Previous: 5df34cb Ratio
MRI Lab/Bloch/CPU/2 thread(s) 238195724.5 ns 223767376.5 ns 1.06
MRI Lab/Bloch/CPU/4 thread(s) 169331898 ns 130477217 ns 1.30
MRI Lab/Bloch/CPU/8 thread(s) 86911436 ns 138275342 ns 0.63
MRI Lab/Bloch/CPU/1 thread(s) 347229312 ns 347398917 ns 1.00
MRI Lab/Bloch/GPU/CUDA 55542254.5 ns 56227105 ns 0.99
MRI Lab/Bloch/GPU/oneAPI 500467925 ns 496678721 ns 1.01
MRI Lab/Bloch/GPU/Metal 551118604 ns 559269812.5 ns 0.99
MRI Lab/Bloch/GPU/AMDGPU 34655907 ns 34678068 ns 1.00
Slice Selection 3D/Bloch/CPU/2 thread(s) 1004510044 ns 1144007818 ns 0.88
Slice Selection 3D/Bloch/CPU/4 thread(s) 572369922 ns 609258944.5 ns 0.94
Slice Selection 3D/Bloch/CPU/8 thread(s) 337693200 ns 378398584 ns 0.89
Slice Selection 3D/Bloch/CPU/1 thread(s) 1950723198 ns 1928663456 ns 1.01
Slice Selection 3D/Bloch/GPU/CUDA 101462465 ns 101329308.5 ns 1.00
Slice Selection 3D/Bloch/GPU/oneAPI 635759355 ns 635643734 ns 1.00
Slice Selection 3D/Bloch/GPU/Metal 553018250 ns 554670167 ns 1.00
Slice Selection 3D/Bloch/GPU/AMDGPU 58719048 ns 58892774 ns 1.00

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

Copy link

codecov bot commented Sep 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.27%. Comparing base (386cf0d) to head (e03607a).
Report is 4 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #486   +/-   ##
=======================================
  Coverage   90.27%   90.27%           
=======================================
  Files          54       54           
  Lines        3055     3055           
=======================================
  Hits         2758     2758           
  Misses        297      297           
Flag Coverage Δ
base 86.24% <100.00%> (ø)
core 93.13% <ø> (ø)
files 91.69% <ø> (ø)
komamri 93.98% <ø> (ø)
plots 91.32% <ø> (ø)

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

Files with missing lines Coverage Δ
KomaMRIBase/src/datatypes/sequence/Grad.jl 87.87% <100.00%> (ø)

Copy link
Member

@cncastillo cncastillo left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@pvillacorta pvillacorta merged commit 356c48a into master Sep 26, 2024
19 checks passed
@pvillacorta pvillacorta deleted the fix-rotation branch September 26, 2024 17:09
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