Support for RGB #22

aquatiko · 2019-08-08T18:03:42Z

I plan to add many data fields which will hold the results of operations performed on AstroImage. All those fields will be added to mutable struct properties.

aquatiko · 2019-08-08T18:07:16Z

If CI passes and there aren't any changes required regarding this approach, then it can be merged.

giordano

Could you please elaborate on what you want to achieve here? It's not clear to me at all and doesn't look like it's particularly efficient

src/AstroImages.jl

codecov-io · 2019-08-09T01:38:10Z

Codecov Report

Merging #22 into master will decrease coverage by 2.18%.
The diff coverage is 81.25%.

@@            Coverage Diff             @@
##           master      #22      +/-   ##
==========================================
- Coverage   99.18%   96.99%   -2.19%     
==========================================
  Files           4        4              
  Lines         122      133      +11     
==========================================
+ Hits          121      129       +8     
- Misses          1        4       +3

Impacted Files	Coverage Δ
src/AstroImages.jl	`95.94% <81.25%> (-4.06%)`	⬇️

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 9a3483d...4b2a7e6. Read the comment docs.

aquatiko · 2019-08-13T02:20:39Z

@giordano Appvyeor needs a re-run.

src/AstroImages.jl

giordano · 2019-08-18T19:26:24Z

src/AstroImages.jl

-AstroImage(data::NTuple{N, Matrix{T}}, wcs::NTuple{N, WCSTransform}) where {T<:Real, N} = AstroImage{T,Gray,N}(data, wcs)
+    AstroImage{T,color, 1, AbstractFloat}((data,), (wcs,), Properties{AbstractFloat}())
+function AstroImage(color::Type{<:Color}, data::NTuple{N, Matrix{T}}, wcs::NTuple{N, WCSTransform}) where {T<:Real, N}
+    if N == 3 && color == RGB && T <: Union{AbstractFloat, FixedPoint}


Why not

if N == 3 && color == RGB if T <: Union{AbstractFloat, FixedPoint} ... else ... end

instead of repeating conditions multiple times?

Actually, why not using multiple method instead of having runtime checks on parameters?

giordano · 2019-08-18T19:26:52Z

src/AstroImages.jl

+        return AstroImage{T,color,N, widen(T)}(data, wcs, Properties{widen(T)}(rgb_image = img))
+    elseif N == 3 && color == RGB
+        img = ccd2rgb((data[1], wcs[1]),(data[2], wcs[2]),(data[3], wcs[3]))
+        return AstroImage{T,color,N, AbstractFloat}(data, wcs, Properties{AbstractFloat}(rgb_image = img))


Why the parameter is abstract here?

@giordano The parameter is Abstract here for the following reasons:

When we fixed the MappedArrays type for rgb_image, the RGB subtype in it can only have P as an AbstractFloat or FixedPoint numbers.

Also, note that we are not filling rgb_image in all cases. So, when I simply passed T as a parameter (as done in the previous commit), it generated errors when T was not AbstractFloat or FixedPoint.

A solution for this was to pass a valid parameter just for the sake of passing it (when rgb_image is not getting filled). That's what is done except in one case where rgb_image is filled and the type isn't a valid one. The type needs to be in the above-mentioned domain as per Images.jl.

giordano · 2019-08-18T19:28:38Z

src/AstroImages.jl

+        return AstroImage{T,color, N, AbstractFloat}(data, wcs, Properties{AbstractFloat}())
+    end
+end
+AstroImage(data::Matrix{T}) where {T<:Real} = AstroImage{T,Gray,1, AbstractFloat}((data,), (WCSTransform(2),), Properties{AbstractFloat}())


Again, why the last parameter of the returned object is abstract?

same reason as above.

giordano · 2019-08-18T19:28:52Z

src/AstroImages.jl

+    end
+end
+AstroImage(data::Matrix{T}) where {T<:Real} = AstroImage{T,Gray,1, AbstractFloat}((data,), (WCSTransform(2),), Properties{AbstractFloat}())
+AstroImage(data::NTuple{N, Matrix{T}}) where {T<:Real, N} = AstroImage{T,Gray,N, AbstractFloat}(data, ntuple(i-> WCSTransform(2), N), Properties{AbstractFloat}())


Same as above

same reason as above.

giordano · 2019-08-18T19:29:12Z

src/AstroImages.jl

+end
+AstroImage(data::Matrix{T}) where {T<:Real} = AstroImage{T,Gray,1, AbstractFloat}((data,), (WCSTransform(2),), Properties{AbstractFloat}())
+AstroImage(data::NTuple{N, Matrix{T}}) where {T<:Real, N} = AstroImage{T,Gray,N, AbstractFloat}(data, ntuple(i-> WCSTransform(2), N), Properties{AbstractFloat}())
+AstroImage(data::Matrix{T}, wcs::WCSTransform) where {T<:Real} = AstroImage{T,Gray,1, AbstractFloat}((data,), (wcs,), Properties{AbstractFloat}())


Abstract parameter

same reason as above.

giordano · 2019-08-18T19:29:25Z

src/AstroImages.jl

+AstroImage(data::Matrix{T}) where {T<:Real} = AstroImage{T,Gray,1, AbstractFloat}((data,), (WCSTransform(2),), Properties{AbstractFloat}())
+AstroImage(data::NTuple{N, Matrix{T}}) where {T<:Real, N} = AstroImage{T,Gray,N, AbstractFloat}(data, ntuple(i-> WCSTransform(2), N), Properties{AbstractFloat}())
+AstroImage(data::Matrix{T}, wcs::WCSTransform) where {T<:Real} = AstroImage{T,Gray,1, AbstractFloat}((data,), (wcs,), Properties{AbstractFloat}())
+AstroImage(data::NTuple{N, Matrix{T}}, wcs::NTuple{N, WCSTransform}) where {T<:Real, N} = AstroImage{T,Gray,N, AbstractFloat}(data, wcs, Properties{AbstractFloat}())


Abstract parameter

same reason as above.

giordano · 2019-08-18T19:29:53Z

test/ccd2rgb.jl

@@ -44,4 +44,11 @@ end
    @test isapprox(red.(asinh_res), red.(asinh_ans), nans = true, rtol = 3e-5)
    @test isapprox(blue.(asinh_res), blue.(asinh_ans), nans = true, rtol = 3e-5)
    @test isapprox(green.(asinh_res), green.(asinh_ans), nans = true, rtol = 3e-5)
+
+    @testset "AstroImage using ccd2rgb" begin


Probably test coverage can be improved for this new feature

This part takes relatively large time compared to other tests. So, I wanted to keep non-redundant tests only (as AstroImage is tested very comprehensively through other tests). Also, in the next PR, I'm adding more fields and already written more tests in this very @testset.

I'm a bit worried by the fact this takes a lot of time. Does it mean that performance is that bad?

Reproject is the bottleneck where it takes around 20 seconds to reproject an image of size 1024*1024.

This doesn't sound like a good news

giordano · 2019-08-20T08:10:25Z

src/AstroImages.jl

@@ -85,9 +85,21 @@ for n in (8, 16, 32, 64)
    end
 end

-struct AstroImage{T<:Real,C<:Color, N}
+mutable struct Properties{P <: Union{AbstractFloat, FixedPoint}}
+    rgb_image::MappedArrays.MultiMappedArray{RGB{P},2,Tuple{Array{P,2},Array{Float64,2},Array{P,2}},Type{RGB{P}},typeof(ImageCore.extractchannels)}


That Float64 should probably be P?

Yeah, missed that

Support for RGB

d34831b

giordano requested changes Aug 8, 2019

View reviewed changes

src/AstroImages.jl Outdated Show resolved Hide resolved

src/AstroImages.jl Outdated Show resolved Hide resolved

parametric inner constructor

bf2cf6a

aquatiko force-pushed the 3Color branch from 6328415 to bf2cf6a Compare August 12, 2019 18:46

giordano requested changes Aug 15, 2019

View reviewed changes

src/AstroImages.jl Outdated Show resolved Hide resolved

src/AstroImages.jl Outdated Show resolved Hide resolved

src/AstroImages.jl Outdated Show resolved Hide resolved

aquatiko added 2 commits August 18, 2019 00:37

MappedArrays dependency

d02ea78

refactoring

afd6a18

aquatiko force-pushed the 3Color branch from 8caf0a4 to afd6a18 Compare August 18, 2019 14:53

giordano requested changes Aug 18, 2019

View reviewed changes

giordano reviewed Aug 20, 2019

View reviewed changes

minor changes

4b2a7e6

aquatiko force-pushed the 3Color branch from 99b507e to 4b2a7e6 Compare August 20, 2019 10:37

giordano merged commit 98bc20d into JuliaAstro:master Aug 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for RGB #22

Support for RGB #22

aquatiko commented Aug 8, 2019 •

edited

Loading

aquatiko commented Aug 8, 2019

giordano left a comment

codecov-io commented Aug 9, 2019 •

edited

Loading

aquatiko commented Aug 13, 2019 •

edited

Loading

giordano Aug 18, 2019

giordano Aug 18, 2019

aquatiko Aug 19, 2019

giordano Aug 18, 2019

aquatiko Aug 19, 2019

giordano Aug 18, 2019

aquatiko Aug 19, 2019

giordano Aug 18, 2019

aquatiko Aug 19, 2019

giordano Aug 18, 2019

aquatiko Aug 19, 2019

giordano Aug 18, 2019

aquatiko Aug 19, 2019 •

edited

Loading

giordano Aug 20, 2019

aquatiko Aug 20, 2019

giordano Aug 20, 2019

giordano Aug 20, 2019

aquatiko Aug 20, 2019

Support for RGB #22

Support for RGB #22

Conversation

aquatiko commented Aug 8, 2019 • edited Loading

aquatiko commented Aug 8, 2019

giordano left a comment

Choose a reason for hiding this comment

codecov-io commented Aug 9, 2019 • edited Loading

Codecov Report

aquatiko commented Aug 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aquatiko Aug 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aquatiko commented Aug 8, 2019 •

edited

Loading

codecov-io commented Aug 9, 2019 •

edited

Loading

aquatiko commented Aug 13, 2019 •

edited

Loading

aquatiko Aug 19, 2019 •

edited

Loading