Minimum of three NoisyChannels iterations?

[a-hurst]

Finally coming back to PyPREP after an extended break (got super-busy with other stuff), and just noticed a weirdness I think might be a bug: during robust re-referencing, after the initial NoisyChannels call, the noisy channel detection loop that's supposed to run a maximum of 4 iterations will actually run a maximum of 5, and requires at least 3 loops to complete before breaking.

The reason for this is that iterations is set to 0 at the start of the loop, but is only incremented at the very end (after the "check if able to break the loop or if too many iterations" part as already happened). I noticed this because for one file, the 'bad_all' for my first two iterations were identical but PyPREP still went on to do a 3rd loop, which seemed weird to me.

Looking at the corresponding MATLAB PREP code, their logic has the same issue: they still allow 5 iterations when the maximum is 4, and require a minimum of 3 RANSAC loops before no change in bad channels can break the loop. However, looking at the pseudocode in the original PREP publication, it doesn't say anything about a minimum number of iterations, just "break from loop if badChannels didn't change or iteration criteria has been met" which suggests that it might be an honest mistake.

My question is: should PyPREP mimic this behaviour? For the purpose of PyPREP vs MATLAB PREP comparison it should probably be left in as a configurable setting (e.g. a matlab_compat flag or something that tells PREP to strictly follow the original logic), but beyond that is there a rationale for this extra iteration I'm not thinking of?

If it's confirmed this is unwanted, I'll put together a PR to fix it!

[sappelhoff]

Hi @a-hurst this is interesting, thanks! Could you perhaps open this as an issue on the Matlab prep repo as well? I'd be interested in what they say. If it's truly a bug we should fix it, like we did the median computation (I think), which was wrong in Matlab prep.

[a-hurst]

Okay, submitted an issue in the original repository. Will let you know when (if) they respond!

EDIT: Just out of curiosity, I went and looked at the git blame around that loop, and I found that the behaviour used to allow breaking after a single iteration, but during part of a massive commit with a bunch of refactoring on a bunch of files (only a month before the initial publication, suggesting this change wasn't implemented at the time of submission), that iterations > 1 qualifier got added in there with no corresponding entry in the changelog (despite other changes in that commit being mentioned). If it was intentional, the change was never documented.

EDIT (SA): crossref: VisLab/EEG-Clean-Tools#26

[sappelhoff]

reading back on this, we should fix it in PyPREP and introduce that matlab_strict flag to:

pyprep/pyprep/reference.py

Line 101 in 8897804

def perform_reference(self, max_iterations=4):