On 13-1-2014 0:56, clipka wrote:
> Am 12.01.2014 22:21, schrieb andrel:

>> I am working with another group that has apparently a different opinion
>> on writing code. Their code has numerous instances of this code snippet
>>
>>      S=getSmode(T,dep,rep, GEOM.SPECS ,4);
>>      PHI=lowpassma(GEOM.AMA*S,5);
>>
>> this is a short version, there are others that do it in one line, take
>> subsets and do other stuff as well:
>>
>>     PSIA =lowpassma(SCAN.AMA * getSmode(ones(length(GEOM.VER),1) * ( 1 :
>> SCAN.qrsduration ),dep,SCAN.rep,
>> GEOM.SPECS,SCAN.scanmode,GEOM),SCAN.lpass);
>>
>> (never mind that most variables have different names in this place for
>> no obvious reason, it still does the same thing. There are many more
>> unacceptable things in this snippet of code, but let's stick to the
>> naming issue here.)
>>
>> Question: what do you think this does?
>
> I have no freaking idea.
>

Isn't it obvious that the S is a description of the local transmembrane 
potentials (TMP) in time? And that with a simple matrix multiplication 
with a transfermatrix that describes the influence of every point on the 
heart to every point on the torso, you can compute an ECG? And that that 
has very high frequency noise because of the coarse mesh at the heart 
surface, so you should filter that with a moving average filter?

The original formulation on the back of the envelop was apparently that

PHI=A*S

Hence the name "getS" for computing TMP from timing. Later various 
versions were introduces that depended on the "mode", even if now only 
mode 4 is used, it still shows. The A in the formula you can find back 
in AMA which is short for A-MAtrix.
IMHO the moving average is not the right type of filter, but you can not 
change that without changing all instances of this call. One of the 
other things that is very wrong here.
And did I complain the "lpass' is in samples not in frequency or even 
milliseconds and that it should be odd (but that that is not 
documented)? And that it is set to 5 in a number of places in the code? 
And that lowpassma has a bug?
And... long story short: I am not happy with the programming skills of 
this group.

>> Would you create a separate function for this, even if it is just one or
>> two lines and how would you name that function?
>
> Absolutely - even though for different reasoning: One of my primary
> personal mantras is that no code sequence should appear more than once
> in any given program - not even (or, especially not!) with slight
> variations.

I fully agree, that is also one of my reasons. For me it is so obvious 
that I would not even mention it. It took me as a surprise that other 
people that consider themselves skilled programmers can produce this code.

<snipped more open doors (that are apparently closed to some)>

yes I fully agree, <sigh> :(

>
> For instance, while refactoring the old POV-Ray code I came across
> plenty of places where old code computes /some/ scalar measure of an RGB
> colour's "intensity": Some just take the value of the brightest colour
> component, i.e. max(r,g,b); others take the greyscale value of the
> colour, i.e. (r*R)+(g*G)+b*B) with some constant R,G,B. Some use
> absolute values, some just keep negative values around. And some add
> some more twists to it.
>
> Why are there so many variations on this theme?
>
> Chances are, at least some of these don't always do what they should do.
> Some will probably break down for special cases, while some may work
> equally well for all cases but are fundamentally unsuited for the
> purpose they're used for.
>
>
> As a first step towards cleaning up all this mess, I ended up with six
> different functions:
>
> (1) A function to be called to compute a colour's greyscale value.
>
> (2) A function to be called to compute how intense the most intense
> colour channel is.
>
> (3) A function to be called to compute some more or less abstract
> "weight" or "importance" from a colour, currently implemented as the
> colour's greyscale value. (This reeks of bogosity, and will probably be
> replaced by (5) later.)
>
> (4) A function to be called to compute some more or less abstract
> "weight" or "importance" from a colour, currently implemented as the
> intensity of the most intense colour channel. (This, too, reeks of
> bogosity, and will probably be replaced by (6) later.)
>
> (5) A function to be called to compute some more or less abstract
> "weight" or "importance" from a colour, currently implemented as the
> /absolute/ of the colour's greyscale value. (Less bogus than (3), but
> ultimately to be revised as well.)
>
> (6) A function to be called to compute some more or less abstract
> "weight" or "importance" from a colour, currently implemented as the
> highest /absolute/ intensity of all the colour channels. (Less bogus
> than (4), but ultimately to be revised as well.)
>

Did I miss something along the lines of 
http://en.wikipedia.org/wiki/Luma_(video) in this list?



-- 
Everytime the IT department forbids something that a researcher deems
necessary for her work there will be another hole in the firewall.

Post a reply to this message