timeflux / timeflux_rasr

Implementation of rASR filtering.
MIT License
26 stars 4 forks source link

Reconstruction matrix blending #4

Open lkorczowski opened 4 years ago

lkorczowski commented 4 years ago

EDIT: Following the 2020-01-15 meeting, we decided to investigate the blending option and see what will be the best way to implement it. Two mains options has been discussed:

EDIT2: Following the #5 and the drop of 2D compatibility, post-RASR blending seems the best option. See https://github.com/bertrandlalo/timeflux_rasr/issues/4#issuecomment-584582494


Hi,

I'm struggling a bit to see if I'm stupid our there are some issues with the current matlab implementation of the the RASR process (transform for python).

First, I can't figure out in asr_process there is this weird "reconstruction to intermediate samples" that doesn't make sense to me.

  % do the reconstruction in intervals of length stepsize (or shorter at the end of a chunk)
        last_n = 0;
        for j=1:length(update_at)
            % apply the reconstruction to intermediate samples (using raised-cosine blending)
            n = update_at(j);
            if ~trivial || ~state.last_trivial
                subrange = range((last_n+1):n);
                blend = (1-cos(pi*(1:(n-last_n))/(n-last_n)))/2;
                data(:,subrange) = bsxfun(@times,blend,R*data(:,subrange)) + bsxfun(@times,1-blend,state.last_R*data(:,subrange));
            end
            [last_n,state.last_R,state.last_trivial] = deal(n,R,trivial);
        end

R being NOT updated in this loop, doing

data(:,subrange) = bsxfun(@times,blend,R*data(:,subrange)) + bsxfun(@times,1-blend,state.last_R*data(:,subrange));

sounds exatly like doing data(:,subrange) = R*data(:,subrange) in a more fancy (and inefficient) way. Did I miss something ?

I think that this line of code was supposed to do some kind of re-sampling interpolation but it doesn't make sense because there is no resampling. Another possibility: it was made to update R in the loop for overlapping windows (when you have to remove the contribution of the already filtered signal in the previous range). Anyway, I'm just putting this here because in the last commit, I considered that all that section was useless.

lkorczowski commented 4 years ago

the only time data(:,subrange) = bsxfun(@times,blend,R*data(:,subrange)) + bsxfun(@times,1-blend,state.last_R*data(:,subrange)); is actually doing something is at the beginning of the loop, that's all (j=1). And I still don't understand why :/

lkorczowski commented 4 years ago

Not as important, but still...

        % use eigenvalues in descending order
        [D, order] = sort(reshape(diag(D),1,C));

is actually in ascending order... (a lot of weird stuff are everywhere like this)

lkorczowski commented 4 years ago

Decision required here: @bertrandlalo @sylvchev @mesca

mesca commented 4 years ago

Still not sure of what "this" exactly refers to.

We should stick to the sklearn API, so in any case let's keep the code in transform(). If the adaptative mode is needed, it should be a boolean parameter of the constructor.

lkorczowski commented 4 years ago

As I understand it, the code

               blend = (1-cos(pi*(1:(n-last_n))/(n-last_n)))/2;
                data(:,subrange) = bsxfun(@times,blend,R*data(:,subrange)) + bsxfun(@times,1-blend,state.last_R*data(:,subrange));

makes an interpolation between the reconstruction of the data with the current reconstruction matrix R and the previous one last_R.

It may smooth the signal when two call of the RASR.transform(). Indeed, the reconstruction matrix R depends directly of the statistics of the signal, therefore each successive call of RASR.transform() gives a different R. Discontinuities will therefore appears at the edge of the epochs and can be smoothed with the blending.

mesca commented 4 years ago

Oh, I remember our discussion now :) I think we should implement it, because of the weird artifacts that will appear otherwise, but I am open to counter-arguments.

lkorczowski commented 4 years ago

OK, I'll implement the blending with a condition if the reconstruction matrix changed.

I'm closing this issue when implemented

sylvchev commented 4 years ago

I'll be a little more cautious about this, since EEGLab did not respond to your comments on the matlab implementation. I think it could be a parameter of transform(), like blending with a default true value to mimick the Matlab code. I have the feeling that we could get rid of this, it would be easier with a change of default behavior.-

lkorczowski commented 4 years ago

Noted:

This behaviour won't be in sprint2 but in sprint3.

lkorczowski commented 4 years ago

Comment:

Due to current behaviour in Timeflux, each epoch = 1 reconstruction matrix. In the case of sliding windows for epoching, several epochs may have same timestamp but different resulting signals after RASR (because the reconstruction matrix changed). Blending such as proposed in the matlab implementation might be a great solution in Timeflux to merge redundant timestamp signals when required. It may be either an option withing RASR or managed outside RASR (e.g. blending node).

lkorczowski commented 4 years ago

Example of transition

Capture d’écran 2020-01-20 à 15 52 08

without(left) or with(right) blending. top: input signal. bottom: blended signal. Alternating between two reconstruction matrices (identity and [-1,1;1,1])

Pro of blending:

Con of blending:

Added to backlog #15

lkorczowski commented 4 years ago

Following #5:

To do so:

PRO:

CON:

I can write down the code of a blending class, sklearn API compatible.