Back to the main page.
Bug 3252 - hpos and vpos are used in two different ways
| Status | NEW |
| Reported | 2017-02-15 11:42:00 +0100 |
| Modified | 2017-03-27 09:45:03 +0200 |
| Product: | FieldTrip |
| Component: | plotting |
| Version: | unspecified |
| Hardware: | PC |
| Operating System: | Mac OS |
| Importance: | P5 normal |
| Assigned to: | Roemer van der Meij |
| URL: | |
| Tags: | |
| Depends on: | |
| Blocks: | |
| See also: |
Roemer van der Meij - 2017-02-15 11:42:45 +0100
In ft_plot_topo/lay it is: % 'hpos' = horizontal position of the lower left corner of the local axes % 'vpos' = vertical position of the lower left corner of the local axes % 'width' = width of the local axes % 'height' = height of the local axes In ft_plot_matrix/others it is: % 'hpos' = horizontal position of the center of the local axes % 'vpos' = vertical position of the center of the local axes % 'width' = width of the local axes % 'height' = height of the local axes (In ft_plot_lay, the code actually follows both styles...) The first is matlab's style, like in e.g. figure positions, 'position', [left bottom width height]. The second is in the style of our layout's: hpos and vpos match pos(:,1) and pos(:,2). I propose we shift all uses of hpos and vpos to the latter. Mostly because it matches the layout, and is the least amount of work (see below). Personally, it also seems a more intuitive reference frame (to think from the center, instead of the lower left edge). The affected ./plotting/ft_plot_XXX are: ip-145-102-26-33:plotting roemervandermeij$ grep "'vpos'\|'hpos'" ./*.m -l ./ft_plot_box.m (center) ./ft_plot_lay.m (broken...both) ./ft_plot_line.m (center) ./ft_plot_matrix.m (center) ./ft_plot_patch.m (center) ./ft_plot_text.m (center) ./ft_plot_topo.m (left/bottom) ./ft_plot_vector.m (center) ./ft_uilayout.m . (left/bottom) An exception to this change, IMO, should be ft_uilayout. This functions serves as an intermediate interface for UI things, and all UI things have 'position''s which follow [left bottom width height]. So, after reading matlab's documentation on UI things, and setting those UI things using ft_uilayout, it makes sense that it is done according to matlab's style. (Also, it will be a TON of work to change this thing.). What do you think Robert/JM? It's inhibiting my work with the layouts, so I'd be happy to do this. Needless to say, it can mess up a lot of plotting, but luckily we have plenty of test scripts to use for to test (e.g. the tutorials, and those below) Functions that use ft_plot_lay: ip-145-102-26-33:fieldtrip-dev roemervandermeij$ grep -R --include \*.m ft_plot_lay ./ -l .//ft_layoutplot.m .//ft_multiplotCC.m .//ft_multiplotER.m .//ft_multiplotTFR.m .//ft_singleplotTFR.m .//ft_topoplotCC.m .//plotting/ft_plot_lay.m .//plotting/ft_select_channel.m .//plotting/test/test_ft_plot_lay.m .//private/topoplot_common.m .//test/inspect_bug2993.m .//test/test_bug2773.m Functions that use ft_plot_topo: ip-145-102-26-33:fieldtrip-dev roemervandermeij$ grep -R --include \*.m ft_plot_topo ./ -l .//fileio/private/ft_hastoolbox.m .//forward/private/ft_hastoolbox.m .//ft_databrowser.m .//ft_movieplotTFR.m .//inverse/private/ft_hastoolbox.m .//plotting/ft_plot_topo.m .//plotting/ft_plot_topo3d.m .//plotting/test/test_ft_plot_topo.m .//plotting/test/test_ft_plot_topo3d.m .//private/moviefunction.m .//private/topoplot_common.m .//realtime/example/ft_realtime_topography.m .//realtime/online_meg/ft_realtime_coillocalizer.m .//test/failed_eeg_leadfield_units.m .//test/failed_headmodel_fns.m .//test/inspect_bug3033.m .//test/test_bug1082.m .//test/test_bug2336.m .//test/test_bug2377.m .//test/test_headmodel_asa.m .//test/test_headmodel_bemcp.m .//test/test_headmodel_concentricspheres.m .//test/test_headmodel_dipoli.m .//test/test_headmodel_openmeeg.m .//test/test_headmodel_singlesphere.m .//test/test_meg_leadfield_units.m .//utilities/ft_hastoolbox.m
Roemer van der Meij - 2017-02-15 11:44:10 +0100
Also, this will impact outside use of ft_plot_topo and lay. Any idea who this will be? SPM I presume?
Roemer van der Meij - 2017-02-15 13:32:45 +0100
There is actually another issue, the scaling of the input to adjust to the pseudo axis is broken quite a bit in ft_plot_topo/lay. This was probably invisible within the same layout, but becomes very obvious using nested layouts. The below makes it obvious. According to the lay (and the input given to ft_plot_topo), the topo plot should go from X = 0.5-.25/2 to X = 0.5+.25/2. It however goes from X = 0.633 to X = 0.883. A significant rightward (same for upward) shift, but accurate width. If the axes of the topo data start at 1 and are positive, the error is very minimal, which might be the reason why it wasn't an issue in SPMs GUI (it's in there right?), as the layout generally follows that principle (but needn't). Not hard to fix, but will again have an impact for other toolboxes I presume. % create lay and plot fake data lay = []; lay.label{1} = 'chan'; lay.pos = [0.5 0.5]; lay.width = 0.25; lay.height = 0.25; figure; axis on; ft_plot_topo(51:100,51:100,rand(50),'hpos',lay.pos(1,1)-lay.width(1)/2,'vpos',lay.pos(1,2)-lay.height(1)/2,'width',lay.width(1),'height',lay.height(1),'hlim',[51 100],'vlim',[51 100]) For comparison, ft_plot_matrix (with hpos/vpos as it desires), is accurate: figure; axis on; ft_plot_matrix(51:100,51:100,rand(50),'hpos',lay.pos(1,1),'vpos',lay.pos(1,2),'width',lay.width(1),'height',lay.height(1),'hlim',[51 100],'vlim',[51 100])
Jan-Mathijs Schoffelen - 2017-03-02 10:12:21 +0100
Hi Roemer, I agree with your diagnosis and suggested fix. But I would be curious about Robert's opinion as well. I wouldn't be too worried about unforeseen changes in behavior of the functions, since it solely affects visualization and not algorithmic computations. In that light, even though we have quite a few test functions calling ft_plot_lay c.s., typically we don't run the function with the intention to visually check the figures. Typically we use them to check whether they continue running through...