ptiede / Comrade.jl

MIT License
48 stars 7 forks source link

Added Unit Wisp Geometrical Model #81

Closed dominic-chang closed 2 years ago

dominic-chang commented 2 years ago

Added model for a Unit Wisp. I hope the area scaling is correct

ptiede commented 2 years ago

I took a look at the implementation today and I think there is a small bug somewhere. The visibility should be equal to the intensity at the origin however I am not getting a unit flux. Instead, I get something ~1.35. I am guessing there is a small missing factor.

edit: I just looked into this and the normalization is off by a factor of arclength/4. If you take the limit as (u,v)->0 I get the visibility is equal to 4/l. So if I change

dominic-chang commented 2 years ago

For the wisp or the parabolic segment? Or both?

ptiede commented 2 years ago

For the wisp or the parabolic segment? Or both?

This is for both. I think the length should drop out here for the Fourier transform so the function should be

function visibility_point(::ParabolicSegment{T}, u, v) where {T}
    ϵ = eps(T)
    vϵ = v + ϵ + 0im
    phase = exp(1im*π*(3/4 + 2*vϵ + u^2/(2vϵ)))
    #length = (√5 + asinh(2)/2)
    Δ1 = erf(√(π/(2vϵ))*exp(π*im/4)*(u-2vϵ))
    Δ2 = erf(√(π/(2vϵ))*exp(π*im/4)*(u+2vϵ))
    return phase/(√(2vϵ))*(Δ1-Δ2)/4
end

This also fixes the confusion about the normalization. There is no sinh issue so the ParabolicSegment and the Wisp should be equal.

edit ignore the prinln

dominic-chang commented 2 years ago

I think the length should drop out here for the Fourier transform so the function should be

image

Yep, your totally right. The normalization factor is actually proportional to "a" and not the length. I made the fallacious assumption that the integral of a delta of a function that describes a level curve is the area of that curve. I just checked it for this wisp and it in fact just "a"

ptiede commented 2 years ago

Alright, I did a rebase off main and fixed the normalization. Given that the Parabolic segment should equal the Wisp I decided to just ax it. Let me know if you object to it!

codecov-commenter commented 2 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (wisp@fcbadd4). Click here to learn what that means. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             wisp      #81   +/-   ##
=======================================
  Coverage        ?   76.56%           
=======================================
  Files           ?       15           
  Lines           ?      798           
  Branches        ?        0           
=======================================
  Hits            ?      611           
  Misses          ?      187           
  Partials        ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update fcbadd4...0cadea6. Read the comment docs.

ptiede commented 2 years ago

Merged! Great job Dominic.