Back to the main page.
Bug 1745 - test_ft_prepare_localspheres.m fails
| Status | CLOSED FIXED |
| Reported | 2012-09-26 15:50:00 +0200 |
| Modified | 2013-06-06 15:09:45 +0200 |
| Product: | FieldTrip |
| Component: | forward |
| Version: | unspecified |
| Hardware: | PC |
| Operating System: | Windows |
| Importance: | P3 normal |
| Assigned to: | Johanna |
| URL: | |
| Tags: | |
| Depends on: | |
| Blocks: | |
| See also: |
Johanna - 2012-09-26 15:50:44 +0200
Related to bug 1414: test-script fails after the update to make ft_headmodel_localspheres more robust (or strict is a better way to say it) to geometry input. However, ft_prepare_headmodel was not updated to go along with this change. ft_prepare_headmodel still creates a geometry structure of size 3 on line 209 in the call to prepare_shells, but then ft_headmodel_localspheres now crashes if given a geometry structure size greater than 1 (line 260 of ft_prepare_headmodel). Where should the change be made? 1) In ft_headmodel_localspheres where if geometry is greater than size 1, then another key-value pair option for selection of which geometry? or 2) in ft_prepare_headmodel in 'case localspheres' (near line 259) prior to call to ft_headmodel_localspheres? or 3) require cfg.tissue to be set by user when calling ft_prepare_headmodel with option cfg.method='localspheres' and 'hasvolume && needbnd' is true?
Johanna - 2012-09-26 15:51:53 +0200
see bug 1679
Johanna - 2012-10-02 17:01:59 +0200
I've taken the new version of ft_prepare_headmodel from Robert and am creating a test_ft_prepare_headmodel, cycling over all cfg.method options in combination with all data input options. I'm stuck on one question for now: How should cfg.radius default be set, when the 'headshape' is in fact a mesh from the brain surface (rather than scalp)? Should this option just be forbidden? or should the call to ft_headmodel_localspheres from ft_prepare_headmodel be smart enough to know to increase the radius and maxradius depending on the segmentation input?
Johanna - 2012-10-19 17:04:17 +0200
Created attachment 348 ft_prepare_headmodel_new.m
Johanna - 2012-10-19 17:05:08 +0200
Created attachment 349 test_ft_prepare_headmodel.m
Johanna - 2012-10-19 17:06:16 +0200
Hi Robert, Can we meet to discuss the 2 files I just attached? I have many questions and comments (related to all the 'FIXME's in the code). Thanks!
Johanna - 2012-10-26 13:27:52 +0200
Hi Robert, The ft_headmodel_(BEM) options currently work such that dipoli and openmeeg can handle a 1-shell vol input, via the "if isolatedsource" option, whereas bemcp does not have this option, and instead, bemcp requires that numel(geometry) must be 3. Is this correct/desired?
Johanna - 2012-11-28 18:26:40 +0100
Robert, in the old (current) version of ft_prepare_headmodel, the call to simbio and fns together looks like: vol = feval(funname,geometry,'tissue',cfg.tissue,'tissueval',cfg.tissueval, ... 'tissuecond',cfg.tissuecond,'sens',cfg.sens, ... 'transform',cfg.transform,'unit',cfg.unit); However, none of these 6 inputs are used by simbio (all by fns). Instead, ft_headmodel_simbio wants 'conductivity' (and it is not given it!). But based on the cfg options in ft_prepare_headmodel, which of these two inputs should it use? vol = ft_headmodel_simbio(geometry,'conductivity',cfg.conductivity); or vol = ft_headmodel_simbio(geometry,'conductivity',cfg.tissuecond);
Robert Oostenveld - 2012-11-29 09:14:29 +0100
(In reply to comment #7) Simbio will need a mesh with tetrahedral or hexahedral elements, where each element will have a scalar or tensor conductivity. I guess that the same applies to FNS. I suggest to - split the two altogether. - use cfg.conductivity in both, pass it as 'tissuecond' to fns and 'conductivity' to simbio - update the help in ft_headmodel_fns to specify the correct options (it now specifies one that is not used, and does not specify any one that is used). I also suggest not to include any tests pertaining to simbio or fns into test_ft_prepare_headmodel. The main reason for the general test of ft_prepare_headmodel is to ensure that it is and remains consistent with the old implementations like ft_prepare_bemmodel. The new implementations that have not been used yet (simbio, fns) are better tested in their respective specific test scripts.
Johanna - 2012-12-04 11:09:27 +0100
Hi Robert, Do you have (or can point me to) an ASA file that will past the test ft_filetype(cfg.hdmfile, 'asa_vol') that can be used for testing in the test_ft_prepare_headmodel?
Johanna - 2012-12-04 11:48:24 +0100
I've noticed in bemcp, dipoli, and openmeeg that if default conductivity values are used, then they are first assigned, then the order of the shells is rearranged if needed to 'inside first'. Shouldn't the re-arranging occur first, and then if no conductivity set by user, then assign default cond values since the order is now certain? Robert, did you have another way to know that the order was predetermined as outside-first (skull scalp brain)?
Johanna - 2012-12-04 13:00:41 +0100
For the use of concentricspheres in ft_prepare_headmodel, we added last week during the bug binge that a headshape can be entered as geometry. This is 1-shell. Should ft_headmodel_concentricspheres then also allow/set a default conductivity if length(vol.r)==1?
Robert Oostenveld - 2012-12-04 13:10:54 +0100
(In reply to comment #9) Sure. I have copied some to the test directory. I believe these were made with ASA version 2, which is a really old version. But since then I have not used ASA any more. roboos@mentat001> pwd /home/common/matlab/fieldtrip/data/test/original roboos@mentat001> ll -d */asa drwxr-xr-x 2 roboos mrphys 4096 Dec 4 12:58 electrodes/asa drwxr-xr-x 2 roboos mrphys 4096 Dec 4 12:57 headmodel/asa drwxr-xr-x 2 roboos mrphys 4096 Dec 4 12:59 headshape/asa drwxr-xr-x 2 roboos mrphys 4096 Dec 4 12:58 mri/asa I also added a test script for the reading. It contains elcfile = '/home/common/matlab/fieldtrip/data/test/original/electrodes/asa/standard_primed.elc'; volfile = '/home/common/matlab/fieldtrip/data/test/original/headmodel/asa/standard.vol'; skinfile = '/home/common/matlab/fieldtrip/data/test/original/headshape/asa/standard_skin_14038.vol'; mrifile = '/home/common/matlab/fieldtrip/data/test/original/mri/asa/standard.mri'; elec = ft_read_sens(elcfile); vol = ft_read_vol(volfile); skin = ft_read_headshape(skinfile); mri = ft_read_mri(mrifile); ---- mac001> svn commit Adding test/test_fileformat_asa.m Sending test/test_headmodel_interpolate.m Transmitting file data .. Committed revision 7076. Hmm, the test_headmodel_interpolate.m should not have been committed with this.
Robert Oostenveld - 2012-12-04 13:18:38 +0100
(In reply to comment #10) correct, first ensure the order, then use the default conductivity (which implies a default order). You could use this one, which is in forward/private: % BOUNDING_MESH determines if a point is inside/outside a triangle mesh % whereby the bounding triangle mesh should be closed. % % [inside] = bounding_mesh(pos, pnt, tri) % ... e.g. isinside = false(nsurface); for i=1:nsurface pos = bnd(i).pnt(1,:); for j=(i+1):nsurface if bounding_mesh(pos, bnd(j).pnt, bnd(j).tri) isinside(i,j) = true; isinside(j,i) = false; end end end and then [dum, order] = sort(sum(isinside, 2)); if dum is unequal to 1:nsurface there is something wrong. order can subsequently be used to sort the surfaces.
Robert Oostenveld - 2012-12-04 13:19:23 +0100
(In reply to comment #11) yes. I suggest to set the one-shell default conductivity to 1.
Johanna - 2012-12-09 17:48:06 +0100
(In reply to comment #13) I found surface_nesting (inside forward/private) which does what you suggest. However, the labels of 'outsidefirst' and 'insidefirst' seem backwards (opposite logic) to me. I would think I'd need to call the code 'insidefirst' in order to actually get the order skin,skull,brain, whereas I'd conceptually call it outsidefirst. The way that ft_headmodel_bemcp calls it however, is consistent: it calls surface_nesting with 'outsidefirst' option, then later ensures that the 3rd boundary is the skin. So the code mathematically behaves ok, but conceptually the label seems backwards.
Johanna - 2012-12-09 18:14:09 +0100
The default conductivity values for 4-shell dipoli and concentricspheres do not match. dipoli has [1 1 1/80 1] * 0.33; %brain, inner-skull, outer-skull, skin concentricspheres has [0.3300 1 0.0042 0.3300]; % brain, csf, skull, skin Which value is correct for the second shell (1 or 0.33)? Which label is correct (csf or inner-skull)?
Johanna - 2012-12-09 20:53:35 +0100
Related to the original title of this bug, test_ft_prepare_localspheres tries to compare the new and old way of using localspheres with cfg.headshape. However, when we (Robert and I) discussed the new ft_prepare_headmodel at the last bug binge, we decided that only singlesphere and concentricspheres would allow a cfg.headshape (but not localspheres). Is that decision still correct? If so, then I guess I should remove that check from the test_ft_prepare_localspheres. If not, then I will change the (new) ft_prepare_headmodel.
Johanna - 2012-12-12 09:43:40 +0100
Created attachment 391 ft_prepare_headmodel_new.m Updated (and finished?) ft_prepare_headmodel_new.m
Johanna - 2012-12-12 09:44:41 +0100
Created attachment 392 test_ft_prepare_headmodel.m
Robert Oostenveld - 2012-12-18 09:55:21 +0100
(In reply to comment #16) [0.3300 1 0.0042 0.3300 for brain, csf, skull, skin is better and consistent with the use elsewhere (e.g. besa).
Johanna - 2012-12-18 10:21:41 +0100
(In reply to comment #8) I just realised I never implemented this part of comment 8(simbio/fns). Working on it now.
Johanna - 2012-12-18 11:33:12 +0100
(In reply to comment #8) Currently, ft_headmodel_fns wants a segmentation ("seg") as input, not a mesh geometry. The segmentation is of an old style (only a 3D matrix) so that within ft_headmodel_fns this is used: mri = []; mri.dim = size(seg); mri.transform = eye(4); mri.seg = uint8(seg); Shall I just keep it like this and modify ft_prepare_headmodel accordingly (by requiring input_seg to be true, and extracting the 'seg' field as input to ft_headmodel_fns)? I have updated the help of ft_headmodel_fns as so, but welcome improvements: % tissuecond = matrix C [9XN tissue types]; where N is the number of % tissues and a 3x3 tensor conductivity matrix is stored % in each column. % tissue = see fns_contable_write % tissueval = match tissues of segmentation input % transform = 4x4 transformation matrix (default eye(4)) % units = string (default 'cm') % sens = sensor information (for which ft_datatype(sens,'sens')==1) % deepelec = used in the case of deep voxel solution % tolerance = scalar (default 1e-8)
Robert Oostenveld - 2012-12-18 11:43:23 +0100
(In reply to comment #22) > Shall I just keep it like this and modify ft_prepare_headmodel accordingly (by > requiring input_seg to be true, and extracting the 'seg' field as input to > ft_headmodel_fns)? Yes, please keep it like this. Probably Lilla will also have a look at this once SIMBIO is completed.
Johanna - 2013-01-14 14:43:02 +0100
Hi Robert, Could you please respond on comment 17? My thought is that cfg.headshape as a filename string should be allowed for input option to localspheres in ft_prepare_headmodel, in the same way that it can be for singlesphere and concentricspheres. The old functionality seems to be that it was possible. However, I agree that geometry.pnt=data.chanpos should not be allowed for localspheres. Thanks!
Robert Oostenveld - 2013-01-14 14:58:28 +0100
(In reply to comment #24) yes, that should be allowed. Please note that Lilla is also working on this. Perhaps you can quickly convene (since you are on the same floor) and arrange who does what to ensure that no conflicts in the code arise because both of you making changes. Robert
Johanna - 2013-01-14 17:20:42 +0100
I think my updated ft_prepare_headmodel is ready. Thus before committing, I'm testing it out on all test functions that call it. Of ones I've looked at so far, most need minor changing to reflect updated allowed/banned inputs.
Johanna - 2013-01-15 14:22:59 +0100
The new ft_prepare_headmodel.m is now committed. I will keep an eye on dashboard just in case. bash-4.1$ svn commit ft_prepare_headmodel.m test/test_ft_prepare_localspheres.m test/test_ft_prepare_headmodel_new.m test/test_ft_prepare_singleshell.m test/test_bug1368.m test/test_bug1042.m test/test_bug1166.m test/test_bug70.m test/test_headmodel_localspheres_new_old.m test/test_bug1029.m test/test_bug686.m forward/ft_headmodel_localspheres.m Sending forward/ft_headmodel_localspheres.m Sending ft_prepare_headmodel.m Sending test/test_bug1029.m Sending test/test_bug1042.m Sending test/test_bug1166.m Sending test/test_bug1368.m Sending test/test_bug686.m Sending test/test_bug70.m Sending test/test_ft_prepare_headmodel_new.m Sending test/test_ft_prepare_localspheres.m Sending test/test_ft_prepare_singleshell.m Sending test/test_headmodel_localspheres_new_old.m Transmitting file data ............ Committed revision 7316.
Johanna - 2013-01-15 14:44:44 +0100
ft_prepare_headmodel updated and original purpose (as well as all other little things that came up along the way) of this bug are resolved.
Robert Oostenveld - 2013-01-15 15:09:25 +0100
note that the dashboard emails are failing currently