Hi Oh

Math-Random
you found a bug that the GaussianGenerator does not use every random number it generates. nice catch.
1. essentialy you changed the method nextGaussianNormalized. you added:
self hasNextGaussian
          ifTrue: [
              [^ nextGaussian] ensure: [ nextGaussian := nil ] ].
if you look at ensure: you notice that this method has the function that an error does not wreak havoc in the code. iow you would certainly win the contest of finding the most exotic use for a rather specialized method. but you could circumvent the specialized time-eating part simply by looking at the implementation of #ensure: and using the important part this way:
self hasNextGaussian
          ifTrue: [r:= nextGaussian. nexGaussion:=nil. ^r] .
2. you inflated the image by adding the method hasNextGaussian. adding methods is sometimes used to split up methods that are unreadably long or so or make code more readable, but the main reason is reusability. hasNextGaussian obviously has no reusability at all. iow you can simplify the above line to
nextGaussian ifNotNil: [r:= nextGaussian. nexGaussion:=nil. ^r] .
and throw away hasNextGaussian.
3. the most important thing: lets just throw away your changes for a moment and look at the original code of nextGaussianNormalized
    | x1 x2 w y1 y2 |
    hasNextGaussian
        ifTrue: [
            hasNextGaussian := false.
            ^ nextGaussian ].
    [
    x1 := 2.0 * rng nextFloat - 1.0.
    x2 := 2.0 * rng nextFloat - 1.0.
    w := x1 * x1 + (x2 * x2).
    w >= 1.0 ] whileTrue.
    w := (-2.0 * w ln / w) sqrt.
    y1 := x1 * w.
    y2 := x2 * w.
    nextGaussian := y2.
    ^ y1
of course you notice that this had the intention to do the same thing your changes did. obviously the only missing part is setting hasNextGaussian to true in the last block. by adding one (!)line of code eg like this:
nextGaussianNormalized
    | x1 x2 w y1 y2 |
    hasNextGaussian
        ifTrue: [
            hasNextGaussian := false.
            ^ nextGaussian ].
    [
    x1 := 2.0 * rng nextFloat - 1.0.
    x2 := 2.0 * rng nextFloat - 1.0.
    w := x1 * x1 + (x2 * x2).
    w >= 1.0 ] whileTrue.
    w := (-2.0 * w ln / w) sqrt.
    y1 := x1 * w.
    y2 := x2 * w.
    nextGaussian := y2.
    hasNextGaussian :=true
    ^ y1

I think adding a flag ivar can be a source of bug, but in this case, the code is really simple, so I wouldn't have to worry about that. I think your change is good. Make a commit!

 

you could have repaired the whole thing or did i overlook something?
the bug was very obviously just a slip of the pen, probably there once was a line 'hasNextGaussian :=true' in the code, perhaps the author took it out for some testing and forgot to put it again in afterwards. every author has his style and if you change other peoples code you could respect their style. Daniel obviously likes to write simple easily readable code and you changed it to a somewhat cumbersome thing.

I don't want to imagine about what happened to the original author head. What is important is it was flawed.

 

Math-Tests-Random
this is a nice fix indeed. a reaction to the fixme comment in a short and precise way. i have to admit that i once also thought one should add a #abs to the (e - m) part and then was just too lazy to do it and i was unsure how high i should set the treshold for the stdev part. i probably would have deleted the standard deviation part in the fixme comment. nice fix!

you other changes of other peoples code are simpler, you changed code, that was working and had no bugs, just so.

Because the original code was not #abs-ing, it was testing one side of inequality only. The test would not catch the problem when e was falsely too small. So I changed it.

For the stdev part, I wasn't sure about how high it should be. But leaving it not testing didn't seem right so chosen arbitrary number that passes the test. I was confident that someone would point me out if it were wrong.

 

Math-Tests-FunctionFit
you changed (A findString: B) > 0 to B search: A.
#search: is part of an extension of regex, which for example is not in the core of squeak. iow this introduces an unnecessary incompability. then there is #includesSubstring: which does the same as #search: just the argument and the receiver are exchanged and you never know when the pharo chiefs decide that one of them is superfluous and depreciate one of them. should search: one day get depreciated the code will not work anymore.

I wasn't aware of #search: is not core method. Replacing it to #includesSubstring: seem right. I'll fix this issue, soon.

 

Math-Tests-FunctionFit
you made an instance var out of a temp var. i occasionally do also this kind of trick, but was that really necessary? if i would not know that code and would look at one test i would see that an inst var is set and would immediately wonder what was the original setting and is this setting used by other methods, iow i would at least look for inialize and setUp methods and waste 5 seconds.

Sorry for your 5 seconds if troubles you, it is my habit of turning tem var into instVar if all the method declares it.

 

to sum it up, imo it is not necessary to leave your footprint on every piece of code in scismalltalk. and if you think it is necessary, it would be nice if you leave the style of the original author intact. Please dont take it personally <smile>, but it is not really motivating to upload further things if one expects that the code and its style will be changed this way, especially if these changes occur for no reasons, as the code works. 

I have no interest in leaving footprint on every piece of code in scismalltalk. I thought those codes were not right  that's all. If someone thinks code preamble is a sign of dominance like dog's pee on walls and tree trunks, he would thank me for hiding the flaws he made.

HwaJong Oh

 

--
You received this message because you are subscribed to the Google Groups "SciSmalltalk" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
For more options, visit https://groups.google.com/d/optout.