Back to the main page.

Bug 3193 - ft_artifact_eog fltpadding default introduces NaNs that avoid filtering

Reported 2016-10-26 15:46:00 +0200
Modified 2021-09-16 14:55:04 +0200
Product: FieldTrip
Component: core
Version: unspecified
Hardware: PC
Operating System: Windows
Importance: P5 normal
Assigned to: Diego Lozano Soldevilla
Depends on: 2732
See also:

Diego Lozano Soldevilla - 2016-10-26 15:46:43 +0200

The issue starts with the cfg.artfctdef.eog.fltpadding default which is 0.1. This parameter introduces a 0.1s padding with NaNs (line 301 in ft_artifact_zvalue) due to ft_fetch_data and the private function preproc.m does not filter the data because it contains NaNs. To reproduce the but, use the dataFIC as follows: cfg=[]; cfg.trl = data.cfg.previous.trl; cfg.artfctdef.eog = []; cfg.artfctdef.eog.bpfilter = 'yes'; cfg.artfctdef.eog.bpfilttype = 'but'; cfg.artfctdef.eog.bpfreq = [8 10]; cfg.artfctdef.eog.bpfiltord = 4; = 'MLF22'; cfg.artfctdef.eog.trlpadding = 0; %% cfg.artfctdef.eog.fltpadding = 0;%% cfg.artfctdef.eog.interactive = 'yes'; cfg.artfctdef.eog.cutoff = 2.5; [cfg, artifact] = ft_artifact_eog(cfg,dataFIC); Suggestion: After ft_fetch_data, we should detect NaNs and use ft_preproc_padding to add the appropriate padtype: 'zero', 'mean', 'localmean', 'edge', 'mirror'

Jan-Mathijs Schoffelen - 2016-10-26 16:18:02 +0200

So, I would assume that this NaN-padding occurs, if the user inputs already preprocessed data? In some way this could then be classified as a user-error, because padding outside the scope of the data is impossible. Yet, I can imagine the people would expect non-nanny behavior. Perhaps we should at least throw a warning in ft_artifact_zvalue if nans are detected at the edges (i.e. due to the fltpadding being non-zero). Also, we may consider addressing this in bug 2732

Diego Lozano Soldevilla - 2016-10-26 16:22:58 +0200

(In reply to Jan-Mathijs Schoffelen from comment #1) I agree: the warning is already implemented in ft_artifact_zvalue: Warning: data contains NaNs, no filtering or preprocessing applied However, the reason why it happened was a bit less logic given that fltpadding is by default 0.1 so we're going to filter out of the data in many cases... Decision: or we change the defaults or the deal with NaN padding better

Jan-Mathijs Schoffelen - 2016-10-26 16:24:39 +0200

Good point. Perhaps we could stick to the default in case data is read from disk, and default to 0 if there's a data argument in the input? Dealing with the NaNs is something that may be required, but can be followed up elsewhere.

Diego Lozano Soldevilla - 2016-10-26 16:46:31 +0200

(In reply to Jan-Mathijs Schoffelen from comment #3) I like your solution Jefe. I could insert a conditional with warning to set padding values to zero with data as input (around line 229 in ft_artifact_zvalue): if hasdata; cfg.artfctdef.zvalue.trlpadding = 0; cfg.artfctdef.zvalue.fltpadding = 0; ft_warning('trlpadding and fltpadding are set to zero to avoid filter problems with NaN, see bug3193 for details'); end If you agree I can make a pull request (I need to train my github disabilities)

Jan-Mathijs Schoffelen - 2016-10-26 16:49:32 +0200

:o). Sounds like a good plan. Good luck with your disabilities...

Diego Lozano Soldevilla - 2016-10-26 17:07:24 +0200

(In reply to Jan-Mathijs Schoffelen from comment #5)

Diego Lozano Soldevilla - 2016-10-27 11:59:10 +0200

(In reply to Diego Lozano Soldevilla from comment #6) I prefer to continue the thread here: Regarding ft_artifact_jump things are a bit different. There's the cfg.padding in addition to the trlpadding, artpadding and fltpadding. In case cfg.padding is different from zero, if ~isfield(cfg.artfctdef.jump,'trlpadding'), cfg.artfctdef.jump.trlpadding = 0.5*cfg.padding; end if ~isfield(cfg.artfctdef.jump,'artpadding'), cfg.artfctdef.jump.artpadding = 0.5*cfg.padding; end if ~isfield(cfg.artfctdef.jump,'fltpadding'), cfg.artfctdef.jump.fltpadding = 0; end I noticed that for ft_artifact_jump only the cfg.artfctdef.jump.fltpadding = 0; is the critical parameter. I'll add this fix in the next pull request

Jan-Mathijs Schoffelen - 2016-10-27 12:04:11 +0200

OK, thanks for looking into this. It seems that you don't need to create a new pull request. If you make (and commit and push) changes in the branch that you are PR'ing, the new commit will be automatically added, I think.

Diego Lozano Soldevilla - 2016-10-27 13:23:06 +0200

(In reply to Jan-Mathijs Schoffelen from comment #8) you're welcome. Thanks for your feedback. I made another pull request from scratch because I didnt know how to add all together

Jan-Mathijs Schoffelen - 2021-09-16 14:54:55 +0200

As far as I can see, this has been addressed. Closing for now.