Updating get_RF_types, get_RF_center, get_Mk #658
Conversation
cncastillo
left a comment
cncastillo
left a comment
There was a problem hiding this comment.
Hi, thanks for the PR. I see some problems with the code, in general you need to take advantage of multiple dispatch. You can change RFs to be a parametric type RF{RFUse} for this and specialized functions based on the type of RFUse. We want to force the user to use Pulseq RFUse if they don't want us to guess the use of each RF with heuristics like alpha > 90.
Look at MATLAB's Pulseq code to check how they deal with this for the calculation of k-space, so we can match behavior.
| get_RF_center(x::RF) = begin | ||
| t = times(x) | ||
| B1 = ampls(x) | ||
| t_center = sum(abs.(B1) .* t) ./ sum(abs.(B1)) | ||
| if x.use != Undefined() && x.center != 0.0 | ||
| t_center = x.center | ||
| else | ||
| t = times(x) | ||
| B1 = ampls(x) | ||
| t_center = sum(abs.(B1) .* t) ./ sum(abs.(B1)) | ||
| end | ||
| return t_center | ||
| end |
There was a problem hiding this comment.
I think the idea is to dispatch based on the RF{RFUse}, and do exactly what we are doing now if the use is Undefined(), and return r.center otherwise. Here you are doing extra computations for the case that the RF center is defined.
| function get_RF_types(seq, t) | ||
| α = get_flip_angles(seq) | ||
| function get_RF_types(seq, t; ex_rf_th = 90.01) | ||
| α = get_flip_angles(seq) |
There was a problem hiding this comment.
Don't add ex_rf_th, we need to force people to use the RFUse. The same logic as before, if RFUse is undefined, we need to do exactly what we were doing before.
Also, the computation of
RF_ex = (α .<= ex_rf_th) .* RF_mask
RF_rf = (α .> ex_rf_th) .* RF_maskdoesnt make sense if the RFUse is defined, and its being computed for all RFs.
| RF_rf = (α .> ex_rf_th) .* RF_mask | ||
| rf_idx = Int[] | ||
| rf_type = Int[] #cambiar a tipo RFUse | ||
| rf_type = Vector{Any}()#Int[] #cambiar a tipo RFUse |
There was a problem hiding this comment.
Using containers of type any is a very bad practice, as it generates inefficient code. Please use RFUse, if RFUse is undefined we do the same as before but replacing the ints 0 and 1 by Excitation() and Refocusing()).
| if rf_type[rf-1] != Undefined() | ||
| if rf_type[rf-1] == Excitation() # Explicit Excitation | ||
| mk[p,i] .-= 0 | ||
| elseif rf_type[rf-1] == Refocusing() # Explicit Refocuse | ||
| mk[p,i] .-= mkf | ||
| else | ||
| mk[p,i] .+= mkf | ||
| end | ||
| elseif rf_type[rf-1] == 0 # Excitation | ||
| mk[p,i] .-= 0 | ||
| elseif rf_type[rf-1] == 1 # Refocuse | ||
| mk[p,i] .-= mkf | ||
| #end | ||
| else |
There was a problem hiding this comment.
You have the logic repeated twice, this doesn't make sense, please only use an rf_type::Vector{RFUse} (like explained in previous comment).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## pulseq1.5.0 #658 +/- ##
===============================================
- Coverage 90.01% 87.83% -2.18%
===============================================
Files 57 57
Lines 3366 3355 -11
===============================================
- Hits 3030 2947 -83
- Misses 336 408 +72
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
2 participants
No description provided.