SinoGeom

[JeffFessler]

Move towards dispatch version of sinogram geometry types.

[codecov]

Codecov Report

Merging #68 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #68      +/-   ##
==========================================
- Coverage   99.76%   99.76%   -0.01%     
==========================================
  Files          48       49       +1     
  Lines        2121     2117       -4     
==========================================
- Hits         2116     2112       -4     
  Misses          5        5              

Impacted Files	Coverage Δ
src/fbp/ellipse_sino.jl	100.00% <ø> (ø)
src/fbp/rect_sino.jl	100.00% <ø> (ø)
src/plot/jim.jl	100.00% <ø> (ø)
src/fbp/image_geom.jl	100.00% <100.00%> (ø)
src/fbp/sino_geom.jl	100.00% <100.00%> (ø)
src/fbp/sino_plot.jl	100.00% <100.00%> (ø)

---

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 0f6ea0c...62e71ad. Read the comment docs.

[johnnychen94]

Nice!

I noticed that all related data are stored in SinoGeom, which is good. One possible caveat of this is that probably not all methods(SinoMoj, SinoPar, and SinoFan) have the same data fields, and when this happens, some additional checks are needed.

I believe it would be better to organize types in the following way:

abstract type SinoGeom end

"`SinoPar()` parallel-beam geometry"
struct SinoPar <: SinoGeom
    SinoPar_arg1
    SinoPar_arg2
end

struct SinoMoj <: SinoGeom
    SinoMoj_arg1
    SinoMoj_arg2
end

function downsample(sg::T, down::Int) where T <: SinoGeom
    T(...)
end

There might be some code redundancy here, but I feel that makes code decoupled in a meaningful and extensible way.

I don't know any detailed meaning of the codes, so this is just some rough suggestion and it could be inappropriate in your case.

[JeffFessler]

Thanks. I had a similar thought but was concerned about the possible code redundancy.
But I've given it a try with this latest commit and it seems reasonable.
Comments welcome on this version.
There are some plots that would be better to be put into a democard I think, btw...

[johnnychen94]

Just some nichpicking comment 😄

function downsample(sg::T, down::Int) where T<:Union{SinoPar, SinoMoj}
    down == 1 ? return sg : T(_downsample(sg, down)...)
end

and similar to sino_geom_over

[johnnychen94]

Yes, sometimes, code redundancy in the most fundamental layer makes it easier to write orthogonal codes at a higher-level and makes it easy to add new things. sometimes it's just a matter of preference.

There are some plots that would be better to be put into a democard I think, btw...

Is there anything I can do before JeffFessler/mirt-demo#1 is fixed?