mne-python icon indicating copy to clipboard operation
mne-python copied to clipboard

[DOC] Update description of 'epoch_threshold' in compute_bridged_electrodes

Open mscheltienne opened this issue 1 year ago • 2 comments

The description of the argument epoch_treshold mentions ed_threshold, which is nowhere to be found. @alexrockhill Am I correct in assuming that it was an old name for the argument lm_cutoff?

mscheltienne avatar Aug 10 '22 20:08 mscheltienne

@alexrockhill Also, the values in the description of the function and in the description of lm_cutoff don't seem to match. Intended? https://github.com/mne-tools/mne-python/blob/c8d8c5f72e6714ae147994effcee8efe04c648f4/mne/preprocessing/_csd.py#L198-L201

and https://github.com/mne-tools/mne-python/blob/c8d8c5f72e6714ae147994effcee8efe04c648f4/mne/preprocessing/_csd.py#L213-L217

mscheltienne avatar Aug 10 '22 20:08 mscheltienne

The description of the argument epoch_treshold mentions ed_threshold, which is nowhere to be found. @alexrockhill Am I correct in assuming that it was an old name for the argument lm_cutoff?

Yes that's correct and that was an oversight, thanks for fixing it.

The local maximum is part of the EEGLAB implementation but is really just extraneous-if the local minimum is at 0, there is no bridging, if not then by definition there's a local max less than that, usually quite close to zero but that doesn't tell you anything new.

alexrockhill avatar Aug 10 '22 21:08 alexrockhill

@alexrockhill Sorry that I did not get back to you earlier on this. The function description is not clear to me and to @ArthurNguyen who reported this to me.

image

I do not get a clear picture of what the function does. Especially, this local maximum, I don't get what it is. Could you detail what you mean by:

The local maximum is part of the EEGLAB implementation but is really just extraneous-if the local minimum is at 0

If we omit this local maximum part, the first sentence becomes clear: it computes an electrical distance matrix and looks for values below 5uV^2 which are indicative of bridging. Is this correct or am I missing a step/information here?

And final point, if the above is correct, then why does the description mention minimum below 5uV^2 while the default value for lm_cutoff is 16 uV^2?

mscheltienne avatar Aug 14 '22 13:08 mscheltienne

I clean-up a bit the reference to remove the double-link when both URL and DOI were provided. I checked each DOI individually to make sure they were correct.

mscheltienne avatar Aug 14 '22 13:08 mscheltienne

If we omit this local maximum part, the first sentence becomes clear: it computes an electrical distance matrix and looks for values below 5uV^2 which are indicative of bridging. Is this correct or am I missing a step/information here?

Spot on in my view.

And final point, if the above is correct, then why does the description mention minimum below 5uV^2 while the default value for lm_cutoff is 16 uV^2?

If there's lots of bridging, the local minimum can actually be quite a bit higher than 5, this was the value I picked to be sure not to cut off any bridging values. Hope that makes sense.

alexrockhill avatar Aug 14 '22 15:08 alexrockhill

OK, then where does the value 5 come from? EEGLAB default?

How about rephrasing as:

First, an electrical distance matrix is computed by taking the pairwise variance between electrodes. Local minimums below ``lm_cutoff`` are indicative of bridging between a pair of electrodes.

mscheltienne avatar Aug 14 '22 15:08 mscheltienne

OK, then where does the value 5 come from? EEGLAB default?

Yes, exactly. Rephrase sounds good.

alexrockhill avatar Aug 14 '22 15:08 alexrockhill

Render: https://output.circle-artifacts.com/output/job/0fadf717-2dcd-4b69-a4a4-9b86b04d04fd/artifacts/0/dev/generated/mne.preprocessing.compute_bridged_electrodes.html?highlight=compute_bridged_electrodes#mne.preprocessing.compute_bridged_electrodes

mscheltienne avatar Aug 14 '22 16:08 mscheltienne

Render: https://output.circle-artifacts.com/output/job/0fadf717-2dcd-4b69-a4a4-9b86b04d04fd/artifacts/0/dev/generated/mne.preprocessing.compute_bridged_electrodes.html?highlight=compute_bridged_electrodes#mne.preprocessing.compute_bridged_electrodes

Great clarification, looks good thanks!

alexrockhill avatar Aug 14 '22 16:08 alexrockhill

Thanks @mscheltienne

larsoner avatar Aug 15 '22 13:08 larsoner