search

HPCE / hpce-2017-cw5

1 stars 6 forks source link

Formula for gaussian_blur #6

Closed malharjajoo closed 6 years ago

malharjajoo commented 6 years ago

Should the formula (below) have a 2 * r * r in the denominator of the exponential ?

image

m8pple commented 6 years ago

Mathematically, yes, it should, but this function came from production code which deviated from the mathematical spec (which is a feature I wanted to keep). So it was empirically fiddled about with by the developers until they got the smoothing that worked well enough for the next stage, then it was fixed as that set of coefficients. As a result, the filter ended up wider than anticipated, and no-one fixed it to make it match the maths.

Though looking at it I'm now actually suspicious of the overall denominator, as that is something that I had to simplify for this coursework - in the original version the normalisation was calculated empirically, but that makes the code longer (plus strongly suggests how to make this much faster, though I assume most people will either know or guess how to do that anyway).

So I'm actually wondering whether it should be r*r -> r on the overall denominator (for the same reason you highlighted).

m8pple commented 6 years ago

You're right - I got the normalisation wrong, but here it kicks in on the bottom rather than the top. So at the moment the filter weight decays slowly as the radius increases, which is not intended (this was handled by the empirical normalisation that I took out).

That would have added another interesting optimisation opportunity, but not something I intended (for very large scale factors the whole thing becomes trivial...)

So I'll change it to:

return exp(- (dx*dx+dy*dy) / (2*r) ) / (2 * 3.1415926535897932384626433832795 * r );

as that is closes to what the original code did and what I intended to do.

Thanks - I expected someone to (quite reasonably) query the "incorrect" definition, but it was actually incorrect anyway...