Back to the main page.
Bug 2532 - Problem in ft_apply_montage
| Status | CLOSED FIXED |
| Reported | 2014-04-11 13:40:00 +0200 |
| Modified | 2014-05-14 20:08:49 +0200 |
| Product: | FieldTrip |
| Component: | core |
| Version: | unspecified |
| Hardware: | PC |
| Operating System: | Windows |
| Importance: | P5 normal |
| Assigned to: | Robert Oostenveld |
| URL: | |
| Tags: | |
| Depends on: | |
| Blocks: | |
| See also: |
Vladimir Litvak - 2014-04-11 13:40:51 +0200
Created attachment 611 Example inputs I get a crash in ft_apply_montage when applying to EEG sensors. Not sure how to fix it. It has to do with changing the number of channels but still trying get channel types with indices based on the old channel number. Example attached. The command is: sens = ft_apply_montage(sens, sensmontage, 'keepunused', 'no'); The error is: Index exceeds matrix dimensions. Error in ft_apply_montage (line 406) sens.chantype = input.chantype(sel2,:);
Jörn M. Horschig - 2014-04-28 17:09:56 +0200
Hi Vladimir, I am naive to the actual implementation of apply_montage, but since this has been around for some weeks, I thought I give it a shot. However, I am pretty confused. The statement in line 406 makes absolutely no sense, the same counts for line 415. The function ft_apply_montage has exactly one output argument, which is called 'input' (very convenient name for an output variable ^^). These lines are the last two things happening in the function and are changing the not existing sens-variable, which is then also not further used. So, there are two errors here: 1) sens should probably be changed to input 2) the indexing by sel2 is wrong, as input.chantype should be indexed according to .labelnew and not, as currently, to .labelorg 1) was introduced together with http://code.google.com/p/fieldtrip/source/detail?spec=svn7112&r=7112 when Robert changed the variable input and output name, but apparently forgot to change the name in those lines 2) shouldn't be an issue at all, since this is already dealt with earlier in the code since revision 8923: http://code.google.com/p/fieldtrip/source/detail?spec=svn8923&r=8923 Therefore I would propose to just delete these lines, but I need an executive decision by the roboos or jansch (added to CC).
Jörn M. Horschig - 2014-04-30 13:58:33 +0200
we discussed this at the FT meeting. Resolution 1) we will extend the definition of the montage with a chantypeold/new field 2) we will extend the definition of the montage with a chanunitold/new field 3) if the montage does not specify chantype, the output will not have the chantype field. Idem for chanunit. 4) if the chantype/unit need to be updated, it is the responsibility of th ecalling code to specify the type and/or unit ad 1 and 2: both old/new should be specified in the montage, or should be absent
Vladimir Litvak - 2014-05-01 12:07:43 +0200
(In reply to Jörn M. Horschig from comment #2) Good. We have something along these lines in some places in SPM already. One thing I don't understand is why you need chantypeold and chanunitold. It's not really necessary for the montage and it should be in the data anyway.
Robert Oostenveld - 2014-05-01 12:15:00 +0200
(In reply to Vladimir Litvak from comment #3) the montage can in principle change the chantype and chanunits. I.e. ICA on EEG should not have eeg chantypes as output, but if you reverse apply the (artifact-pruned) mixing matrix the result is again interpretable as eeg. Right now ft_apply_montage is unaware of chantype and chanunit, which means that they might become inconsistent with the actual data after calling ft_apply_montage. ... oh, I guess that I only now really understand your question: It is specifically about the old representations of type/unit, not about the new ones. You are right, the old representation is in the input data and would not have to be present in the montage. so this would be the desired structure: % montage.tra = MxN matrix % montage.labelorg = Nx1 cell-array % montage.labelnew = Mx1 cell-array % montage.chantypenew = Mx1 cell-array % montage.chanunitnew = Mx1 cell-array
Vladimir Litvak - 2014-05-01 12:17:30 +0200
(In reply to Robert Oostenveld from comment #4) Yes, that's what I meant.
Vladimir Litvak - 2014-05-09 17:03:37 +0200
(In reply to Vladimir Litvak from comment #5) Hi guys, I see the bug that causes a crash is still there and there was no fix. Should I just comment those lines out for now? The bug might affect our SPM course demos. Vladimir
Robert Oostenveld - 2014-05-11 08:55:46 +0200
*** Bug 2416 has been marked as a duplicate of this bug. ***
Robert Oostenveld - 2014-05-11 09:06:59 +0200
I just started looking at this bug. I realise that with inverse=yes, org and new swap their roles (either for label/chantype/chanunit). Hence both need to be present.
Robert Oostenveld - 2014-05-11 10:28:36 +0200
I have made ft_apply_montage fully aware of chantype and chanunit. They are treated just like labels, i.e. with org and new. In case the input (data or montage) don't contain them, the output will also not contain it. roboos@mentat001> svn commit Sending forward/ft_apply_montage.m Transmitting file data . Committed revision 9480. I can imagine that there will be some places in other existing code or in (test) scripts where the chantype and chanunits now will disappear. This should not be considered as a bug, but as a feature. In case the chantype and units are not explicitly specified, they are unknown and therefore not to be trusted. However, there are cases where they could be specified, e.g. in megplanar (T/cm?). I will file a separate bug for improving the channel unit handling in ft_apply_montage.
Robert Oostenveld - 2014-05-11 11:18:59 +0200
I think for now that we are done. To be followed up as bug 2570.