Back to the main page.
Bug 1128 - collapse multiple calls of ft_checkconfig into one
| Status | CLOSED FIXED |
| Reported | 2011-11-09 10:40:00 +0100 |
| Modified | 2012-08-23 14:02:08 +0200 |
| Product: | FieldTrip |
| Component: | core |
| Version: | unspecified |
| Hardware: | PC |
| Operating System: | Windows |
| Importance: | P4 enhancement |
| Assigned to: | Jörn M. Horschig |
| URL: | |
| Tags: | |
| Depends on: | |
| Blocks: | |
| See also: |
Jörn M. Horschig - 2011-11-09 10:40:21 +0100
improves readability&cleanness see e.g ft_freqstatistics: % check if the input cfg is valid for this function cfg = ft_checkconfig(cfg, 'renamed', {'approach', 'method'}); cfg = ft_checkconfig(cfg, 'required', {'method'}); cfg = ft_checkconfig(cfg, 'forbidden', {'transform'}); this can be replaced by only one call % check if the input cfg is valid for this function cfg = ft_checkconfig(cfg, 'renamed', {'approach', 'method'}, 'required', {'method'}, 'forbidden', {'transform'});
Robert Oostenveld - 2011-11-09 12:44:56 +0100
but multiple renames don't work like this. I prefer the "one at a time check", but let's vote at the FT meeting
Boris Reuderink - 2011-11-17 10:46:45 +0100
Changed the status of bugs without a specific owner to UNCONFIRMED. I'll try to replicate these bugs (potentially involving the submitter), and change confirmed bugs to NEW. Boris
Boris Reuderink - 2011-11-17 13:47:19 +0100
I remember that we agreed on something.. but I forgot what it actually was. Jörn, could you formulate what we agreed on? Boris
Robert Oostenveld - 2011-11-17 14:13:19 +0100
we agreed to keep the multiple 'renamed' and 'renamedval' calls separate. The multiple 'required' calls should be merged, just like the multiple 'forbidden' calls.
Jörn M. Horschig - 2011-11-17 15:03:44 +0100
Afair, we agreed to collapse all calls that can be collapsed per type. i.e. per function only once these calls: cfg = ft_checkconfig(cfg, 'required', {aaa,bbb,ccc,...}) cfg = ft_checkconfig(cfg, 'forbidden', {ddd,eee,fff,...}) However, for 'renamed, one call per renamed field needs to be made, because you cannot collapse this into one call: cfg = ft_checkconfig(cfg, 'renamed', {xxx, zzz}); cfg = ft_checkconfig(cfg, 'renamed', {vvv, www}); cfg = ft_checkconfig(cfg, 'renamed', {..., ...}); One of the plotting function (multi- or topoplot) is an excellent function where this should be changed ;)
Jörn M. Horschig - 2011-11-17 15:04:25 +0100
I should read all my mails+scroll down before answering ;)
Robert Oostenveld - 2011-11-17 16:05:36 +0100
but your description still beats mine ;-)
Jörn M. Horschig - 2012-03-13 14:07:17 +0100
I am currently changing some function calls, and saw that in ft_volumelookup, there is such a call: ft_checkconfig(cfg, 'forbidden', {'sphere' 'box'}, ... 'required', {'atlas' 'inputcoord'}); Still sure that we don't want all calls to be more like this? :) It may be microtuning, but it would remove unnecessary double checks, and I would volunteer to do that. For now, I just committed what we agreed upon: svn ci -m "enhancement-#1128-collapsed multiple ft_checkconfig calls to one" Sending ft_clusterplot.m Sending ft_multiplotER.m Sending ft_multiplotTFR.m Sending ft_prepare_mesh.m Sending ft_prepare_mesh_new.m Sending ft_singleplotER.m Sending ft_singleplotTFR.m Sending ft_spikesorting.m Sending ft_spiketriggeredaverage.m Sending ft_spiketriggeredinterpolation.m Sending ft_spiketriggeredspectrum.m Sending ft_topoplotTFR.m Sending ft_volumelookup.m Sending statistics_montecarlo.m Sending test/test_ft_megplanar.m Transmitting file data ............... Committed revision 5449.
Robert Oostenveld - 2012-03-13 14:23:17 +0100
(In reply to comment #8) for some options it would remain readable, for some others it would not. E.g. I would not know how to do two times "renamedvalue" in a single call. In the example you give it is indeed appropriate to combine them in one call. You may want to look at roboos@mentat001> for file in ft_*m ; do echo `grep ft_checkconfig $file | wc -l` $file ; done or roboos@mentat001> for file in ft_*m ; do echo `grep ft_checkconfig $file | wc -l` $file ; done | sort -n to find the ones that have most calls to ft_checkconfig. Note the 36 in ft_topoplotTFR!