Added @materialize convenience DSL

[jagot]

As sketched in https://github.com/JuliaApproximation/ContinuumArrays.jl/pull/37

[codecov[bot]]

Codecov Report

Merging #91 into master will increase coverage by 0.73%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #91      +/-   ##
==========================================
+ Coverage   77.81%   78.54%   +0.73%     
==========================================
  Files          14       15       +1     
  Lines        1172     1212      +40     
==========================================
+ Hits          912      952      +40     
  Misses        260      260

Impacted Files	Coverage Δ
src/LazyArrays.jl	66.66% <ø> (ø)	:arrow_up:
src/materialize_dsl.jl	100% <100%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5e45747...f7829a8. Read the comment docs.

[jagot]

I assume the intermittent CI failures (e.g. https://travis-ci.org/JuliaArrays/LazyArrays.jl/jobs/633648027#L305) are related to https://github.com/JuliaArrays/LazyArrays.jl/pull/90#issuecomment-568234141?

[dlfivefifty]

I think this was fixed in master though perhaps I missed some?

[jagot]

What do you think about the idea? @tkf any input?

[tkf]

My initial thought was "can this be done without a macro?" but I can't think of any idea. I guess I can see that a macro like this could be handy.

Regarding DSL syntax, I'd probably try to be slightly more verbose, something like

@materialize function *(Ac::Adjoint{<:Any,<:AbstractMatrix},
                        O::MyOperator,
                        B::AbstractMatrix)
    ApplyStyle(_, _) = MyApplyStyle()
    function similar(_, T)
        A = parent(Ac)

        if O.kind == :diagonal
            Diagonal(Vector{T}(undef, O.n))
        else
            Tridiagonal(Vector{T}(undef, O.n-1),
                        Vector{T}(undef, O.n),
                        Vector{T}(undef, O.n-1))
        end
    end
    function copyto!(dest::Diagonal, _)
        dest.diag .= 1
    end
    function copyto!(dest::Tridiagonal, _)
        dest.dl .= -2
        dest.d .= 1
        dest.du .= 3
    end
end

so that you can kind of guess the meaning a bit more without reading what macro does.

Another idea is to make it super generic and replace __op__ and __applied__ anywhere in the function arguments. Like this:

@materialize function *(Ac::Adjoint{<:Any,<:AbstractMatrix},
                        O::MyOperator,
                        B::AbstractMatrix)
    LazyArrays.ApplyStyle(__op__, typeof(__applied__)) = MyApplyStyle()
    function Base.similar(__applied__, T)
        ...
    end
    function Base.copyto!(dest::Diagonal, __applied__)
        ...
    end
    function Base.copyto!(dest::Tridiagonal, __applied__)
        ...
    end
    Broadcast.materialize(__applied__) =
        copyto!(similar(__applied__, eltype(__applied__)), __applied__)
end

Also, I wonder if there is a better name for this. Maybe @deflazy? @deflazy function ... can be read "define lazy function."

[jagot]

Thanks @tkf for your input! Sorry for not responding earlier, I was preoccupied preparing a talk.

I agree that @materialize sounds like the macro will perform the materialization, rather than setting up the functions. How about @defmaterialize? @deflazy is a bit too general?

I also agree it's better to be a bit verbose and less magical, but I don't like the underscores, nor the __applied__; all the arguments the user will need are specified in the macro invoction. How about the middle ground?:

@defmaterialize function *(Ac::Adjoint{<:Any,<:AbstractMatrix},
                           O::MyOperator,
                           B::AbstractMatrix)
    ApplyStyle = MyApplyStyle()
    function similar(T) # or perhaps even `similar where T`?
        A = parent(Ac)

        if O.kind == :diagonal
            Diagonal(Vector{T}(undef, O.n))
        else
            Tridiagonal(Vector{T}(undef, O.n-1),
                        Vector{T}(undef, O.n),
                        Vector{T}(undef, O.n-1))
        end
    end
    function copyto!(dest::Diagonal)
        dest.diag .= 1
    end
    function copyto!(dest::Tridiagonal)
        dest.dl .= -2
        dest.d .= 1
        dest.du .= 3
    end
end

I specifically don't want to write

Broadcast.materialize(__applied__) =
        copyto!(similar(__applied__, eltype(__applied__)), __applied__)

since it's going to be the same every time.

[tkf]

I specifically don't want to write

Broadcast.materialize(__applied__) =
        copyto!(similar(__applied__, eltype(__applied__)), __applied__)

since it's going to be the same every time.

I think this is an indication that macro-based solution is not quite right for defining Broadcast.materialize. I think it's better to provide an abstract apply style (say) CopyToApplyStyle with

Broadcast.materialize(A::Applied{<:CopyToApplyStyle}) =
    copyto!(similar(A, eltype(A)), A)

This way, you can just write

struct MyApplyStyle <: CopyToApplyStyle end

@materialize function *(Ac::Adjoint{<:Any,<:AbstractMatrix},
                        O::MyOperator,
                        B::AbstractMatrix)
    LazyArrays.ApplyStyle(__op__, typeof(__applied__)) = MyApplyStyle()
    ...
    # no `materialize` here
end

all the arguments the user will need are specified in the macro invoction

It means that users need to know exactly what functions are supported by the macro. Documenting everything accurately is cumbersome and reading everything to understand is hard for users. It is burdensome to do this for every API update. I think the generic macro I suggested is better because there are very few concepts to explain.

function similar(T) # or perhaps even `similar where T`?

I think you should at least get the arity correct because similar's signature is complicated. That's why there are _s in my first suggestion.

[jagot]

I think this is an indication that macro-based solution is not quite right for defining Broadcast.materialize. I think it's better to provide an abstract apply style (say) CopyToApplyStyle with

Broadcast.materialize(A::Applied{<:CopyToApplyStyle}) =
    copyto!(similar(A, eltype(A)), A)

Fair enough, that makes sense.

all the arguments the user will need are specified in the macro invoction

It means that users need to know exactly what functions are supported by the macro. Documenting everything accurately is cumbersome and reading everything to understand is hard for users. It is burdensome to do this for every API update. I think the generic macro I suggested is better because there are very few concepts to explain.

Again, makes sense, but the copyto! case actually generates a bit of boilerplate, which is inserted before (size-checking and destructuring the arguments) and after (actually returning the materialized dest object). The insertion of this boilerplate, as well as the generation of the Applied type for the function signatures, is the whole point of this DSL (for me at least). An __applied__/__op__ substitution mechanism allows specifying any function (which is cool and I guess, as you say, more futureproof), but then you would have to special-case the generation of copyto!, which in some sense feels more magical. Or you do special-case copyto!, and if the user performs the size-checks him/herself, then you wind up with duplicated code, which isn't a disaster:

@materialize function *(Ac::Adjoint{<:Any,<:AbstractMatrix},
                        O::MyOperator,
                        B::AbstractMatrix)
    LazyArrays.ApplyStyle(__op__, typeof(__applied__)) = MyApplyStyle()
    function Base.similar(__applied__, T)
        ...
    end
    function Base.copyto!(dest::Diagonal, __applied__)
        axes(dest) == axes(__applied__) || throw(DimensionMismatch("axes must be same"))
        Ac,O,B = __applied__.args
    
        dest.diag .= -1 # Actually only required line
        dest
    end
end

# Generation result
function Base.copyto!(dest::Diagonal, __applied__::Applied{...})
    # Boilerplate
    axes(dest) == axes(__applied__) || throw(DimensionMismatch("axes must be same"))
    Ac,O,B = __applied__.args

    # Boilerplate ends, user code
    axes(dest) == axes(__applied__) || throw(DimensionMismatch("axes must be same"))
    Ac,O,B = __applied__.args
    
    dest.diag .= -1
    dest

    # User code ends, boilerplate resumes
    dest
end

Gist of the idea: good to be general, but is it fine to be magical for copyto!?

function similar(T) # or perhaps even `similar where T`?

I think you should at least get the arity correct because similar's signature is complicated. That's why there are _s in my first suggestion.

Also fair enough.

[tkf]

Again, makes sense, but the copyto! case actually generates a bit of boilerplate, which is inserted before (size-checking and destructuring the arguments) and after (actually returning the materialized dest object).

I think making the destructuring automatic for functions taking __applied__ as an argument is nice. I think this behavior makes total sense, is well-defined, and is still very generic.

The size-checking is a bit of problem though. If taking the idea "do less with macros as much as possible" very seriously, it makes sense to add

function unsafe_copyto! end

function Base.copyto!(dest, A::Applied{<:CopyToApplyStyle})
    axes(dest) == axes(A) || throw(DimensionMismatch("axes must be same"))
    return unsafe_copyto!(dest, A)
end

in LazyArrays.jl so that users can do

struct MyApplyStyle <: CopyToApplyStyle end

@materialize function *(Ac::Adjoint{<:Any,<:AbstractMatrix},
                        O::MyOperator,
                        B::AbstractMatrix)
    LazyArrays.ApplyStyle(__op__, typeof(__applied__)) = MyApplyStyle()
    function LazyArrays.unsafe_copyto!(dest::Diagonal, __applied__)
        ...code touching dest, Ac, O, and B...
    end
    ...
end

Gist of the idea: good to be general, but is it fine to be magical for copyto!?

I still think it's a bit too DWIM (the size-checking part). At the same time, one can ask if adding LazyArrays.unsafe_$f for every Base.$f is the right solution for automating size-checking.

But at least I like the fact that CopyToApplyStyle and the macro are completely orthogonalized in the unsafe_copyto!-based design.

[jagot]

I think I actually like that approach; we don't have to add LazyArrays.unsafe_$f for all things we can think of, since if with the general approach you've proposed, the user could implement any function this way. If there is some boilerplate that arises, a new ApplyStyle could be defined and the same pattern can be repeated, but that does not necessarily need to go into LazyArrays.jl.

I will implement a new version of the macro along these lines soonish.

[tkf]

we don't have to add LazyArrays.unsafe_$f for all things we can think of

Right, I agree.

I will implement a new version of the macro along these lines soonish.

Sounds great :+1:

@dlfivefifty Are you OK with this approach? i.e., the second approach (with __applied__) I proposed in https://github.com/JuliaArrays/LazyArrays.jl/pull/91#issuecomment-573366820 combined with CopyToApplyStyle https://github.com/JuliaArrays/LazyArrays.jl/pull/91#issuecomment-576014919 and unsafe_copyto! https://github.com/JuliaArrays/LazyArrays.jl/pull/91#issuecomment-576983603.

[dlfivefifty]

I have no strong opinions so once you both agree feel free to merge

[jagot]

Note to self: don't forget to add Core.@__doc__