Back to the main page.
Bug 3193 - ft_artifact_eog fltpadding default introduces NaNs that avoid filtering
| Status | CLOSED FIXED |
| 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 |
| URL: | |
| Tags: | |
| Depends on: | 2732 |
| Blocks: | |
| See also: | http://bugzilla.fieldtriptoolbox.org/show_bug.cgi?id=3238 |
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; cfg.artfctdef.eog.channel = '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) https://github.com/fieldtrip/fieldtrip/pull/242
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