feat:add modes to conv function (#399)

[Longhao-Chen]

The realization of #399

[galenlynch]

I think this is a reasonable feature to add. Ideally we'd only do the convolution for the requested output, but until we can implement that I think doing this extra copy is reasonable. I think we should mention that options besides :full will result in an extra copy of the result, which can cause non-trivial memory usage when convolving large arrays.

It's not clear to me if the code, as it is, would work with ND arrays. It seems like you're only doing the first two dimensions.

I don't understand why you made new function names for each of the modes, could the copying be implemented in the method that accepts the mode keyword argument? I would prefer to not rename conv's methods unless it's necessary for some reason.

The method chain for conv is a bit of a mess right now, but it's not clear to me that you could use mode keyword argument with all of the cases that are now supported by conv.

[galenlynch]

Why only apply this to the first two dimensions?

[Longhao-Chen]

I always think that n-dimensional convolution only needs to be cut on the first two dimensions.

It may take me a few days to have free time to fix it.

[Longhao-Chen]

I don't understand why you made new function names for each of the modes, could the copying be implemented in the method that accepts the mode keyword argument? I would prefer to not rename conv's methods unless it's necessary for some reason.

OK, I'll revise it in a few days.

The method chain for conv is a bit of a mess right now, but it's not clear to me that you could use mode keyword argument with all of the cases that are now supported by conv.

Could you explain all of the cases that are now supported by conv?

[Longhao-Chen]

I made some changes to the previous problem. I think it can merge now.

[cbrnr]

I'd try to avoid the copy by always computing the :full solution, and if mode === :same or mode === :valid I'd return just the portion of the array as required by the mode argument. Would that work?

[cbrnr]

Additional comment: I would prefer to use strings for the three possible parameter values, i.e. "full", "same", and "valid". AFAIK symbols seem to be a bit misused for this purpose because they look cool, but they can be confusing for newcomers (see e.g. discussion in https://discourse.julialang.org/t/when-should-a-function-accept-a-symbol-as-an-argument/43510).

[martinholters]

Using Symbols in these situations has the advantage that their comparison is cheaper and they participate in constant-propagation, so the selected mode might be used by the compiler for optimizations. While I don't know whether any of these matter in this particular case, it's a good reason for some functions to prefer Symbols over Strings for cases like this, and as a matter of consistency, I then strongly prefer to use Symbols for all arguments that select from a limited number of possibilities (i.e. instead of an enum that would be overkill), even for functions that don't benefit too much themselves.

[cbrnr]

What performance difference do you expect for comparing an argument once? According to what I read (especially the SO answer by @StefanKarpinski), using symbols as arguments only make sense when strings won't work, e.g. type templates.

I've also searched for the use of symbols as arguments in the standard library, but there are none AFAIK. So it's not really clear why people started using that.

Please do not interpret my comments here as criticism, if you want to use symbols in DSP.jl then by all means go ahead and do it. I just want to understand the reasons to do so, and so far I have not heard a compelling argument except when strings do not work. I also don't understand your arguments:

Using Symbols in these situations has the advantage that their comparison is cheaper and they participate in constant-propagation, so the selected mode might be used by the compiler for optimizations.

What do you mean by "constant-propagation" and which optimizations can the compiler use?

While I don't know whether any of these matter in this particular case, it's a good reason for some functions to prefer Symbols over Strings for cases like this,

But if none of these matter why is it still a good reason then? There are reasons to use strings instead (easier for newcomers).

and as a matter of consistency, I then strongly prefer to use Symbols for all arguments that select from a limited number of possibilities

Consistency with what? It's not used in the standard library. It's also not used in DSP.jl, or is it?

(i.e. instead of an enum that would be overkill)

I agree. But one could use strings.

[martinholters]

I've also searched for the use of symbols as arguments in the standard library, but there are none AFAIK. So it's not really clear why people started using that.

A very prominent example from Base is the head field of Expr, but most users rarely get in touch with it. Another one:

julia> printstyled("Hello world!", color=:blue)
Hello world!
julia> printstyled("Hello world!", color="blue")
ERROR: TypeError: in keyword argument color, expected Union{Int64, Symbol}, got a value of type String

But from a very brief look, it's indeed not as common as I thought it would be. OTOH, are Strings more common for this in Base or stdlib?

What do you mean by "constant-propagation" and which optimizations can the compiler use?

julia> f(x::Symbol) = x == :a ? "a" : 0
f (generic function with 1 method)

julia> f(x::String) = x == "a" ? "a" : 0
f (generic function with 2 methods)

julia> g() = (f(:a), f("a"))
g (generic function with 1 method)

julia> @code_warntype g()
MethodInstance for g()
  from g() in Main at REPL[3]:1
Arguments
  #self#::Core.Const(g)
Body::Tuple{String, Union{Int64, String}}
1 ─ %1 = Main.f(:a)::Core.Const("a")
│   %2 = Main.f("a")::Union{Int64, String}
│   %3 = Core.tuple(%1, %2)::Core.PartialStruct(Tuple{String, Union{Int64, String}}, Any[Core.Const("a"), Union{Int64, String}])
└──      return %3

Note that f(:a) is inferred as constant while f("a") is not even type-stable. (Yes, that's a very contrived example, but you get the idea.)

[cbrnr]

A very prominent example from Base is the head field of Expr, but most users rarely get in touch with it.

This is what symbols were made for - if you're doing metaprogramming then symbols are absolutely necessary, I don't doubt that.

Another one: printstyled

Yes! I didn't know that!

But from a very brief look, it's indeed not as common as I thought it would be. OTOH, are Strings more common for this in Base or stdlib?

Such arguments are apparently pretty uncommon in general, for example I found open(command, mode::AbstractString, stdio=devnull) but then also Unicode.normalize(s::AbstractString, normalform::Symbol).

Note that f(:a) is inferred as constant while f("a") is not even type-stable. (Yes, that's a very contrived example, but you get the idea.)

OK. But I doubt that this matters in any real-world (or even contrived) use case.

So in summary, parameters accepting a limited number of values are pretty uncommon, and Julia Base as well as the standard library use both variants, so unfortunately it's not even consistent there. I think as long as a package is consistent it doesn't matter what is used, but I'd always keep in mind that many users coming from other languages will be confused by the use of symbols.

[Longhao-Chen]

First, I noticed that xcorr using Symbols, so I thank conv should also use Symbols.

they can be confusing for newcomers

Is it possible to accept both Symbols and Strings as arguments?

I'd return just the portion of the array as required by the mode argument.

Return SubArray?

[cbrnr]

First, I noticed that xcorr using Symbols, so I thank conv should also use Symbols.

You're right, I didn't see that. Then yes, to be consistent both functions should accept either symbols or strings, but I wouldn't do both (but again, that's not my call).

Return SubArray?

I guess? conv_res[outsize] does copy the array, right? I'm rather new to Julia, but in Python slicing a NumPy array would create a view and not a copy.