apply review suggestions

Co-authored-by: Tim Holy <[email protected]>

Author: johnnychen94

src/ImageBase.jl

@@ -26,7 +26,6 @@ include("diff.jl")
 include("restrict.jl")
 include("utils.jl")
 include("statistics.jl")
-include("compat.jl")
 include("deprecated.jl")
 
 if VERSION >= v"1.4.2" # work around https://github.com/JuliaLang/julia/issues/34121

src/compat.jl

@@ -7,15 +7,17 @@ using ImageCore.MappedArrays: of_eltype
 
 """
 Although stored as an array, image can also be viewed as a function from discrete grid space
-Zᴺ to continuous space R (or C if it is complex value). This module provides the discrete
+Zᴺ to continuous space R if it is gray image, to C if it is complex-valued image
+(MRI rawdata), to Rᴺ if it is colorant image, etc.
+This module provides the discrete
 version of gradient-related operators by viewing image arrays as functions.
 
 This module provides:
 
 - forward/backward difference [`fdiff`](@ref) are the Images-flavor of `Base.diff`
 - gradient operator [`fgradient`](@ref) and its adjoint via keyword `adjoint=true`.
-- divergence operator [`fdiv`](@ref) is the negative sum of the adjoint gradient operator of
-  given vector fields.
+- divergence operator [`fdiv`](@ref) computes the sum of discrete derivatives of vector
+  fields.
 - laplacian operator [`flaplacian`](@ref) is the divergence of the gradient fields.
 
 Every function in this module has its in-place version.
@@ -184,10 +186,11 @@ flaplacian(X::AbstractArray) = flaplacian!(similar(X, maybe_floattype(eltype(X))
 
 The in-place version of the laplacian operator [`flaplacian`](@ref).
 
-!!! tips Non-allocating
-    This function will allocate a new set of memories to store the intermediate
-    gradient fields `∇X`, if you pre-allcoate the memory for `∇X`, then this function
-    will use it and is thus non-allcating.
+!!! tip Avoiding allocations
+    The two-argument method will allocate memory to store the intermediate
+    gradient fields `∇X`. If you call this repeatedly with images of consistent size and type,
+    consider using the three-argument form with pre-allocated memory for `∇X`,
+    which will eliminate allocation by this function.
 """
 flaplacian!(out, X::AbstractArray) = fdiv!(out, fgradient(X))
 flaplacian!(out, ∇X::Tuple, X::AbstractArray) = fdiv!(out, fgradient!(∇X, X))
@@ -206,7 +209,7 @@ Mathematically, the adjoint operator ∂ᵢ' of ∂ᵢ is defined as `<∂ᵢu,
 
 See also the in-place version [`fgradient!(X)`](@ref) to reuse the allocated memory.
 """
-function fgradient(X::AbstractArray{T,N}; adjoint=false) where {T,N}
+function fgradient(X::AbstractArray{T,N}; adjoint::Bool=false) where {T,N}
     fgradient!(ntuple(i->similar(X, maybe_floattype(T)), N), X; adjoint=adjoint)
 end
 
@@ -218,12 +221,15 @@ The in-place version of (adjoint) gradient operator [`fgradient`](@ref).
 The input `∇X = (∂₁X, ∂₂X, ..., ∂ₙX)` is a tuple of arrays that are similar to `X`, i.e.,
 `eltype(∂ᵢX) == eltype(X)` and `axes(∂ᵢX)  == axes(X)` for all `i`.
 """
-function fgradient!(∇X::NTuple{N, <:AbstractArray}, X; adjoint=false) where N
+function fgradient!(∇X::NTuple{N, <:AbstractArray}, X; adjoint::Bool=false) where N
     all(v->axes(v) == axes(X), ∇X) || throw(ArgumentError("All axes of vector fields ∇X and X should be the same."))
     for i in 1:N
         if adjoint
             # the negative adjoint of gradient operator for forward difference is the backward difference
+            # see also
+            # Getreuer, Pascal. "Rudin-Osher-Fatemi total variation denoising using split Bregman." _Image Processing On Line_ 2 (2012): 74-95.
             fdiff!(∇X[i], X, dims=i, rev=true)
+            # TODO(johnnychen94): ideally we can get avoid flipping the signs for better performance.
             @. ∇X[i] = -∇X[i]
         else
             fdiff!(∇X[i], X, dims=i)

src/diff.jl

@@ -152,6 +152,7 @@ end
         19   -8   20  -17   -5  -18  -10
     ]
     ΔX = ref_laplacian(X)
+    # Base.diff doesn't promote Int to floats so we should probably do the same for laplacian
     @test eltype(ΔX) == Int
     @test ΔX_ref == ΔX