Docstrings Review #297
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,17 +8,17 @@ | |
| seq = Sequence(GR::Array{Grad,1}, RF::Array{RF,1}) | ||
| seq = Sequence(GR::Array{Grad,1}, RF::Array{RF,1}, A::ADC, DUR, DEF) | ||
|
|
||
| The Sequence struct contains events of an MRI sequence. Most field names, except for the DEF | ||
| field, consist of matrices or vectors. In these matrices, each column index represents a | ||
| sequence block. This struct serves as input for the simulation. | ||
|
|
||
| # Arguments | ||
| - `GR`: (`::Matrix{Grad}`) gradient matrix. Rows for x-y-z amplitudes and columns are fo blocks | ||
| - `RF`: (`::Matrix{RF}`) RF matrix. The 1 row is for the coil and columns are for blocks | ||
| - `ADC`: (`::Array{ADC,1}`) ADC block vector | ||
| - `DUR`: (`::Vector`, `[s]`) duration block vector | ||
| - `DEF`: (`::Dict{String, Any}`) dictionary with relevant information of the sequence. | ||
| Possible keys include [`"AdcRasterTime"`, `"GradientRasterTime"`, `"Name"`, `"Nz"`, | ||
| `"Num_Blocks"`, `"Nx"`, `"Ny"`, `"PulseqVersion"`, `"BlockDurationRaster"`, | ||
| `"FileName"`, `"RadiofrequencyRasterTime"`] | ||
|
|
||
|
|
@@ -80,17 +80,7 @@ Sequence(GR::Array{Grad,1}, RF::Array{RF,1})= Sequence(reshape(GR, :, 1), reshap | |
| Sequence(GR::Array{Grad,1}, RF::Array{RF,1}, A::ADC, DUR, DEF)= Sequence(reshape(GR, :, 1), reshape(RF, 1, :), [A], [DUR], DEF) | ||
| Sequence() = Sequence([Grad(0, 0)]) | ||
|
|
||
| # Display on the REPL | ||
| Base.show(io::IO, s::Sequence) = begin | ||
| compact = get(io, :compact, false) | ||
| if !compact | ||
|
|
@@ -140,13 +130,13 @@ size(x::Sequence) = size(x.GR[1,:]) | |
|
|
||
| """ | ||
| y = is_ADC_on(x::Sequence) | ||
| y = is_ADC_on(x::Sequence, t::AbstractVecOrMat}) | ||
|
|
||
| Tells if the sequence `seq` has elements with ADC active, or active during time `t`. | ||
|
|
||
| # Arguments | ||
| - `x`: (`::Sequence`) sequence struct | ||
| - `t`: (`::AbstractVecOrMat`, `[s]`) time to check | ||
|
|
||
| # Returns | ||
| - `y`: (`::Bool`) boolean that tells whether or not the ADC in the sequence is active | ||
|
|
@@ -170,13 +160,13 @@ end | |
|
|
||
| """ | ||
| y = is_RF_on(x::Sequence) | ||
| y = is_RF_on(x::Sequence, t::Vector{Real}) | ||
|
|
||
|
Comment on lines
162
to
164
There was a problem hiding this comment. Rename Rename Rename Rename Rename There was a problem hiding this comment. Addressed in 949850c |
||
| Tells if the sequence `seq` has elements with RF active, or active during time `t`. | ||
|
|
||
| # Arguments | ||
| - `x`: (`::Sequence`) Sequence struct | ||
| - `t`: (`::AbstractVector{Real}`, `[s]`) time to check | ||
|
|
||
| # Returns | ||
| - `y`: (`::Bool`) boolean that tells whether or not the RF in the sequence is active | ||
|
|
@@ -286,7 +276,7 @@ end | |
| """ | ||
| T = dur(x::Sequence) | ||
|
|
||
| The total duration of the sequence in seconds. | ||
|
|
||
| # Arguments | ||
| - `x`: (`::Sequence`) Sequence struct | ||
|
|
@@ -306,7 +296,7 @@ always zero, and the final time corresponds to the duration of the sequence. | |
| - `seq`: (`::Sequence`) Sequence struct | ||
|
|
||
| # Returns | ||
| - `T0`: (`::Vector{Real}`, `[s]`) start times of the blocks in a sequence | ||
| """ | ||
| get_block_start_times(seq::Sequence) = cumsum([0; seq.DUR], dims=1) | ||
|
|
||
|
|
@@ -360,16 +350,14 @@ Generates a trapezoidal waveform vector. | |
|
|
||
| # Arguments | ||
| - `A`: (`::Real`) amplitude | ||
| - `t`: (`::Matrix{Real}`, `[s]`) times to evaluate (it's a `1-row matrix`) | ||
| - `ΔT`: (`::Real`, `[s]`) time duration of the top-flat | ||
| - `ζ1`: (`::Real`, `[s]`) rise time duration | ||
| - `ζ2`: (`::Real`, `[s]`) fall time duration | ||
| - `delay`: (`::Real`, `[s]`) delay time | ||
|
|
||
| # Returns | ||
| - `y`: (`::Matrix{Real}`) trapezoidal waveform (it's a `1-row matrix`) | ||
| """ | ||
| ⏢(A, t, ΔT, ζ1, ζ2, delay) = begin | ||
| if sum(abs.(A)) != 0 && ΔT+ζ1+ζ2 != 0 # If no event just ignore calculations | ||
|
|
@@ -454,15 +442,16 @@ end | |
| """ | ||
| B1, Δf_rf = get_rfs(seq::Sequence, t) | ||
|
|
||
| Returns the amplitude and frequency difference with respect to the resonance frequency | ||
| contained in a sequence structure. | ||
|
|
||
| # Arguments | ||
| - `seq`: (`::Sequence`) Sequence struct | ||
| - `t`: (`1-row ::Matrix{Real}`, `[s]`) time points | ||
|
|
||
| # Returns | ||
| - `B1`: (`1-row ::Matrix{Complex}}`, `[T]`) vector of RF pulses | ||
| - `Δf_rf`: (`1-row ::Matrix{Real}`, `[Hz]`) delta frequency vector | ||
| """ | ||
| get_rfs(seq::Sequence, t) = begin | ||
| ϵ = MIN_RISE_TIME | ||
|
|
@@ -487,22 +476,22 @@ Returns all the flip angles of the RF pulses in the sequence `x`. | |
| - `x`: (`::Sequence`) Sequence struct | ||
|
|
||
| # Returns | ||
| - `y`: (`::Vector{Real}`, `[deg]`) flip angles | ||
| """ | ||
| get_flip_angles(x::Sequence) = get_flip_angle.(x.RF)[:] | ||
|
|
||
| """ | ||
| rf_idx, rf_type = get_RF_types(seq, t) | ||
|
|
||
| Get the RF centers and types (excitation or precession). Useful for k-space calculations. | ||
|
|
||
| # Arguments | ||
| - `seq`: (`::Sequence`) Sequence struct | ||
| - `t`: (`::Vector{Real}`, `[s]`) time values | ||
|
|
||
| # Returns | ||
| - `rf_idx`: (`::Vector{Integer}`) indices of the RF centers | ||
| - `rf_type`: (`::Vector{Integer}`, opts: [`0`, `1`]) RF type (`0`: excitation, `1`: | ||
| precession) | ||
| """ | ||
| function get_RF_types(seq, t) | ||
|
|
@@ -536,13 +525,15 @@ Outputs the designed k-space trajectory of the Sequence `seq`. | |
| - `seq`: (`::Sequence`) Sequence struct | ||
| - `Δt`: (`::Real`, `=1`, `[s]`) nominal delta time separation between two time samples | ||
| for ADC acquisition and Gradients | ||
| - `skip_rf`: (`::Vector{Bool}`, `=zeros(Bool, sum(is_RF_on.(seq)))`) boolean vector | ||
| indicating if the RFs should be considered for the k-space computation, with a length | ||
| equal to the number of active RF blocks in a sequence | ||
|
|
||
| # Returns | ||
| - `kspace`: (`3-column ::Matrix{Real}`) k-space | ||
| - `kspace_adc`: (`3-column ::Matrix{Real}`) k-space sampled at ADC times | ||
| """ | ||
| get_kspace(seq::Sequence; Δt=1, skip_rf=zeros(Bool, sum(is_RF_on.(seq)))) = begin | ||
| t, Δt = get_variable_times(seq; Δt) | ||
| Gx, Gy, Gz = get_grads(seq, t) | ||
| G = Dict(1=>Gx, 2=>Gy, 3=>Gz) | ||
|
|
@@ -594,10 +585,13 @@ Outputs the designed M1 of the Sequence `seq`. | |
| - `seq`: (`::Sequence`) Sequence struct | ||
| - `Δt`: (`::Real`, `=1`, `[s]`) nominal delta time separation between two time samples | ||
| for ADC acquisition and Gradients | ||
| - `skip_rf`: (`::Vector{Bool}`, `=zeros(Bool, sum(is_RF_on.(seq)))`) boolean vector | ||
| indicating if the RFs should be considered for the M1 computation, with a length | ||
| equal to the number of active RF blocks in a sequence | ||
|
|
||
| # Returns | ||
| - `M1`: (`3-column ::Matrix{Real}`) first moment | ||
| - `M1_adc`: (`3-column ::Matrix{Real}`) first moment sampled at ADC times | ||
| """ | ||
| get_M1(seq::Sequence; Δt=1, skip_rf=zeros(Bool, sum(is_RF_on.(seq)))) = begin | ||
| t, Δt = get_variable_times(seq; Δt) | ||
|
|
@@ -654,8 +648,8 @@ Outputs the designed M2 of the Sequence `seq`. | |
| for ADC acquisition and Gradients | ||
|
|
||
| # Returns | ||
| - `M2`: (`3-column ::Matrix{Real}`) second moment | ||
| - `M2_adc`: (`3-column ::Matrix{Real}`) second moment sampled at ADC times | ||
| """ | ||
| get_M2(seq::Sequence; Δt=1) = begin | ||
| t, Δt = get_variable_times(seq; Δt) | ||
|
|
@@ -695,16 +689,16 @@ end | |
| """ | ||
| SR, SR_adc = get_slew_rate(seq::Sequence; Δt=1) | ||
|
|
||
| Outputs the slew rate of the Sequence `seq`. | ||
|
|
||
| # Arguments | ||
| - `seq`: (`::Sequence`) Sequence struct | ||
| - `Δt`: (`::Real`, `=1`, `[s]`) nominal delta time separation between two time samples | ||
| for ADC acquisition and Gradients | ||
|
|
||
| # Returns | ||
| - `SR`: (`3-column ::Matrix{Real}`) slew rate | ||
| - `SR_adc`: (`3-column ::Matrix{Real}`) slew rate sampled at ADC times | ||
| """ | ||
| get_slew_rate(seq::Sequence; Δt=1) = begin | ||
| t, Δt = get_variable_times(seq; Δt) | ||
|
|
@@ -740,17 +734,17 @@ end | |
| """ | ||
| EC, EC_adc = get_eddy_currents(seq::Sequence; Δt=1, λ=80e-3) | ||
|
|
||
| Outputs the Eddy currents of the Sequence `seq`. | ||
|
|
||
| # Arguments | ||
| - `seq`: (`::Sequence`) Sequence struct | ||
| - `Δt`: (`::Real`, `=1`, `[s]`) nominal delta time separation between two time samples | ||
| for ADC acquisition and Gradients | ||
| - `λ`: (`::Real`, `=80e-3`, `[s]`) Eddy current decay constant time | ||
|
|
||
| # Returns | ||
| - `EC`: (`3-column ::Matrix{Real}`) Eddy currents | ||
| - `EC_adc`: (`3-column ::Matrix{Real}`) Eddy currents sampled at ADC times | ||
| """ | ||
| get_eddy_currents(seq::Sequence; Δt=1, λ=80e-3) = begin | ||
| t, Δt = get_variable_times(seq; Δt) | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General comment (major revision).
Please make sure that the documentation is not more complex that the function; if the function is:
f(x)=x^2, it does not need a docstring:This is true for the
showfunctions and others, go one-by-one and ask the question "Is this docstring unnecessarily complex for the function?". I would argue that the only functions that need more in-depth documentation are the exported functions. If there is more docstring that code, we have a problem; do not bloat the code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that should be docstring when it is not necessary.
I did the excercise you proposed, however I couldn't find more autodescriptive functions which wouldn't require a docstring, according to my criteria.
For example, this one:
This function looks like is simple enough to don't require a docstring, however the units are important and the direction of the rotation too.
Maybe, I'm a little bit biased since I also try to keep uniformity in the docsstrings.
I eliminated the docstrings for
Base.showmethods in b1f8e54