Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scaling in sigma inGaussian wrong #20

Open
roflmaostc opened this issue Apr 23, 2024 · 15 comments
Open

Scaling in sigma inGaussian wrong #20

roflmaostc opened this issue Apr 23, 2024 · 15 comments

Comments

@roflmaostc
Copy link
Member

julia> gaussian((10,1), sigma=1)
10×1 IndexFunArray{Float64, 2, IndexFunArrays.var"#g#290"{Float64, Tuple{Int64, Int64}, Tuple{Float64, Float64}, Int64}}:
 3.726653172078671e-6
 0.00033546262790251185
 0.011108996538242306
 0.1353352832366127
 0.6065306597126334
 1.0
 0.6065306597126334
 0.1353352832366127
 0.011108996538242306
 0.00033546262790251185

julia> gaussian((10,1), sigma=0.1, scale=0.1)
10×1 IndexFunArray{Float64, 2, IndexFunArrays.var"#g#290"{Float64, Tuple{Int64, Int64}, Tuple{Float64, Float64}, Int64}}:
 5.166420632838008e-55
 1.8048513878454407e-35
 2.862518580549414e-20
 2.0611536224385653e-9
 0.006737946999085473
 1.0
 0.006737946999085473
 2.0611536224385653e-9
 2.862518580549414e-20
 1.8048513878454407e-35

julia> gaussian((10,1), sigma=sqrt(0.1), scale=0.1)
10×1 IndexFunArray{Float64, 2, IndexFunArrays.var"#g#290"{Float64, Tuple{Int64, Int64}, Tuple{Float64, Float64}, Int64}}:
 3.726653172078671e-6
 0.00033546262790251185
 0.011108996538242306
 0.1353352832366127
 0.6065306597126334
 1.0
 0.6065306597126334
 0.1353352832366127
 0.011108996538242306
 0.00033546262790251185
roflmaostc added a commit that referenced this issue Apr 23, 2024
@roflmaostc
Copy link
Member Author

Offset still has the issue that it's not scaled properly.

So the offset is in pixel coordinates instead of scaled coordinates

@roflmaostc
Copy link
Member Author

I'm not sure if this is easily fixable

x_expr8 = :(exp(.- sum(abs2.(x .- offset).*scale))) # scale is 1/(2 sigma^2)

@RainerHeintzmann
Copy link
Member

Well.... offset is defined in pixels always like this in the core functions, which is one way to define it:

julia> rr((5,5), scale=2.0, offset=(1,2))
5×5 IndexFunArray{Float64, 2, IndexFunArrays.var"#g#110"{Float64, Tuple{Int64, Int64}, Tuple{Float64, Float64}, Int64}}:
 2.0      0.0  2.0      4.0       6.0
 2.82843  2.0  2.82843  4.47214   6.32456
 4.47214  4.0  4.47214  5.65685   7.2111
 6.32456  6.0  6.32456  7.2111    8.48528
 8.24621  8.0  8.24621  8.94427  10.0

maybe we should document that better?

@roflmaostc
Copy link
Member Author

Yeah I realized that later.
But wouldn't it be handy to have them in the scaled coordinates?

@roflmaostc
Copy link
Member Author

I guess both has it's pros and cons

@RainerHeintzmann
Copy link
Member

There could be a second optional argument "scaled_offset", but then I never had trouble with this since the offset is mostly used with the default options which work OK.

@roflmaostc
Copy link
Member Author

Yeah in my case I wanted to place a gaussian 10 micrometer away from another one...

@roflmaostc
Copy link
Member Author

Please check this commit

That was still a bug, wasn't it?
Now at least if you divide sigma by 10 and also the scaling, it results in the same gaussian

@roflmaostc
Copy link
Member Author

My commit was the intended fix.
Do you think before was correct?

You can test the current master branch

@RainerHeintzmann
Copy link
Member

... you are right. The commit fixes it. The confusing thing is that the expression is (abs2.(pos .- offset)) .* scale which makes scale not really a linear scale. Thanks for the fix!
An alternative would be to move scale inside in this expression: abs2(.(pos .- offset).* scale ) and replace the factors appropriately. This might make the code easier to understand?

@RainerHeintzmann
Copy link
Member

... since this is a breaking change, we should update the second digit of the version number, right?

@roflmaostc
Copy link
Member Author

It's a bug fix, so I would leave the number the same.

@roflmaostc
Copy link
Member Author

But it's also breaking.
So maybe bumping second digit

@RainerHeintzmann
Copy link
Member

Yup, I prefer the second digit. After all all Software using "gaussian" with a scale not 1.0 will need to update sigma to get the same result. Can you bumb and release?

@roflmaostc
Copy link
Member Author

Should we revert this, or not?

468faaf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants