Smooth function uses ImageFiltering in a strange way

[akabla]

The current code is below. Is there a reason why we load ImageFiltering in this way? I assume the invokelatest are required because we load the package in the function itself. Why don't we import ImageFiltering in RHEOS.jl like the rest of the packages, and get rid of the invokelatest calls?

function smooth(self::RheoTimeData, τ::Real; pad::String="reflect")

    @eval import ImageFiltering: imfilter, Kernel

    # get sample-rate and Gaussian kernel (std. dev)
    samplerate = 1.0/getsampleperiod(self.t)
    Σ = getsigma(τ, samplerate)

    # smooth signal and return
    check = rheotimedatatype(self)
    if (check == strain_only)
        epsilon = Base.invokelatest(imfilter, self.ϵ, Base.invokelatest(Kernel.reflect, Base.invokelatest(Kernel.gaussian, (Σ,))), pad)
        sigma = [];
    elseif (check == stress_only)
        sigma = Base.invokelatest(imfilter, self.σ, Base.invokelatest(Kernel.reflect, Base.invokelatest(Kernel.gaussian, (Σ,))), pad)
        epsilon = [];
    elseif (check == strain_and_stress)
        sigma = Base.invokelatest(imfilter, self.σ, Base.invokelatest(Kernel.reflect, Base.invokelatest(Kernel.gaussian, (Σ,))), pad)
        epsilon = Base.invokelatest(imfilter, self.ϵ, Base.invokelatest(Kernel.reflect, Base.invokelatest(Kernel.gaussian, (Σ,))), pad)
    end

...

end

[moustachio-belvedere]

We did it that way because (at that time, and I think still unless you added some other uses for it?) it was only ever used in that one function, and it seemed a waste of time to load it in with RHEOS every single time, when it's only used infrequently for smoothing, and smoothing itself is rarely time critical so the invokelatest overhead is not important.

At that time precompilation and just startup in general was much slower so I was keen to remove dependencies brought in everytime RHEOS was loaded. It may not be so important now because I've seen Julia team is working hard to improve startup times -- and maybe there's another dependency we already import that has similar functionality so we can get rid of it. I don't mind at all.

(In case it wasn't clear, when you dynamically import within a function you are one world age behind so you have to use invokelatest.)

[akabla]

Thanks, @moustachio-belvedere . That makes sense.
Maybe we can switch to a function in DSP instead, so that we can reduce the list of required packages.

I'll keep this open until we can explore this properly. All is fine with the current approach anyway.

[akabla]

This may be relevant:
JuliaDSP/DSP.jl#112

function smooth(x::Vector, window_len::Int=7, window::Symbol=:lanczos)
    w = getfield(DSP.Windows, window)(window_len)
    return DSP.filtfilt(w ./ sum(w), [1.0], x)
end

https://docs.juliadsp.org/stable/filters/#DSP.Filters.filtfilt
Various windows are available:
https://docs.juliadsp.org/stable/windows/

[akabla]

A draft function smooth2 exists on the dev branch for testing.

The current smooth function takes a timescale, rather than a number of sample, to define the kernel. That seems to be the right way to do it, although it only makes sense for uniform sampling. We need to find an equivalent approach for DSP.

The padding seems to be by default a reflection on either side of the data. Not sure that there is an equivalent of the :circular padding as in the current function. Circular data is not very relevant to the convolution methods anyway as we assume a proper start time. So maybe that's not too bad.

[moustachio-belvedere]

Looks good. Is it problematic that it doesn't support variable sample rate? I thought we might deprecate nonuniform for the majority of RHEOS post-import functions anyway?

I noticed your smooth function uses getsigma defined in base so I guess you solved the issue of timescale interface?

[akabla]

getsigma works fine for your Gaussian kernel, but it may not be the right approach for an arbitrary kernel using DSP. The padding is not quite the same between the two functions, and I'm not sure what DSP does. I like to idea of being able to set the padding as required. For now, I find the original function more appropriate. So this remains open until we can figure out which is best with good benchmarks.

Regarding variable sampling rate, I think we are still in the "not recommended" situation. Apart for the convolution integrals, there is nothing wrong with non-uniform sampling. For instance we should expect freq data to be log sampled. I think we may want to allow the use of smoothing on this data too.

[moustachio-belvedere]

Ah ok I understand now, true it only works for Gaussian.

I think anyone who wants to do something more fancy than Gaussian probably knows what they are doing enough that they can do it themself, so maybe we could just provide some generic interface between their smoothing function and RHEOS structs? With our Gaussian and getsigma as the default. I don't mind if you want to code up more smoothing kernels yourself, I just don't think it's essential.

Maybe we should have a separate issue to track discussion on variable sampling status? Good point on Freq data, I had not considered smoothing logspaced series. I do wonder if time and Freq data should be handled in fundamentally different ways though.