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

add GenericImage and GenericRGBImage alias #80

Merged
merged 7 commits into from
May 15, 2019
Merged

add GenericImage and GenericRGBImage alias #80

merged 7 commits into from
May 15, 2019

Conversation

johnnychen94
Copy link
Member

Cont. #76

GenericImage is more general than AbstractArray{<:Colorant}, the latter doesn't catch AbstractArray{<:Number}

  • PixelLike
  • GenericImage
  • GenericRGBImage
  • RGB2dImage

* PixelLike
* GenericImage
* GenericRGBImage
* RGB2dImage
@codecov
Copy link

codecov bot commented May 13, 2019

Codecov Report

Merging #80 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #80   +/-   ##
=======================================
  Coverage   66.17%   66.17%           
=======================================
  Files           9        9           
  Lines         408      408           
=======================================
  Hits          270      270           
  Misses        138      138
Impacted Files Coverage Δ
src/ImageCore.jl 61.11% <ø> (ø) ⬆️

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 3d8e1a9...b00b0be. Read the comment docs.

@codecov
Copy link

codecov bot commented May 13, 2019

Codecov Report

Merging #80 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #80   +/-   ##
=======================================
  Coverage   66.17%   66.17%           
=======================================
  Files           9        9           
  Lines         408      408           
=======================================
  Hits          270      270           
  Misses        138      138
Impacted Files Coverage Δ
src/ImageCore.jl 61.11% <ø> (ø) ⬆️

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 3d8e1a9...88c093c. Read the comment docs.

src/ImageCore.jl Outdated Show resolved Hide resolved
@timholy
Copy link
Member

timholy commented May 14, 2019

I'm a little bothered by an explosion of names that do not yet have obvious value. Perhaps some illustrations showing what they make better?

@johnnychen94
Copy link
Member Author

johnnychen94 commented May 14, 2019

These three seems quite redundant according to my use experiences

const RealLike{T<:Real} = NumberLike{T}
const FloatLike{T<:AbstractFloat} = RealLike{T}
const FractionalLike{T<:Union{FixedPoint, AbstractFloat}} = RealLike{T}

They appear to be some kind of short alias in the beginning, for example, in ImageDistances

function colwise(dist::PreMetric,
                 a::AbstractMatrix{<:GenericImage},
                 b::AbstractMatrix{<:GenericImage})
    n = get_common_ncols(a, b)
    r = Vector{result_type(dist, a, b)}(undef, n)
    colwise!(r, dist, a, b)
end

This annotation is more readable than

colwise(dist::PreMetric,
                 a::AbstractMatrix{<:AbstractArray{Union{<:Colorant, <:Number}}},
                 b::AbstractMatrix{<:AbstractArray{Union{<:Colorant, <:Number}}})

Another advantage I didn't check yet, is to provide a similar power of traits. Hypothetically speaking,

# ImageMesh.jl
struct Pixel{T<:Point, CT<:Colorant}
    pos::T
    value::CT
end

# AnotherPackage.jl
using ImageCore
using ImageMesh

const GenericImage = Union{ImageCore.GenericImage, AbstractArray{<:Pixel}}

foo(img::GenericImage) # extend the GenericImage

However, I'm not very certain of it.

@timholy
Copy link
Member

timholy commented May 14, 2019

These three seems quite redundant according to my use experiences

That's perhaps the point: why add types that are not actually necessary for anything? The complexity of a project is not entirely independent of length(names(PackageName)). Prior to #76, it looks like NumberLike was widely used, FloatLike and FractionalLike weren't used at all, and RealLike existed only in a couple of isolated places in Images.jl and probably could have used NumberLike instead. That doesn't seem to have changed since they were added, arguing that perhaps the new types weren't necessary.

This annotation is more readable than

colwise(dist::PreMetric,
                 a::AbstractMatrix{<:AbstractArray{Union{<:Colorant, <:Number}}},
                 b::AbstractMatrix{<:AbstractArray{Union{<:Colorant, <:Number}}})

I see value in these:

const NumberLike = Union{Number,AbstractGray}
const Pixel = Union{Number,Colorant}
const GenericGrayImage{T<:NumberLike,N} = AbstractArray{T,N}
const GenericImage{T<:Pixel,N} = AbstractArray{T,N}

I just don't understand why we can't stop there. (I can also live with Generic2dImage, though note it's only 3 characters shorter than GenericImage{T,2}. I know we have AbstractVector and AbstractMatrix, but in linear algebra those correspond to important concepts, whereas in image processing dimensionality is only rarely important from a conceptual standpoint.) I really don't want to get to Generic2dAbstractRGBImageWithN0f8OrN0f16ElementType.

In general I'd add new names on a need-to-have basis.

src/ImageCore.jl Outdated Show resolved Hide resolved
This solves the conceptual issue:

```julia
julia> Colorant <: Pixel
false

julia> Colorant <: Pixel{T} where T
true

julia> Colorant{<:FixedPoint} <: Pixel
true

julia> AbstractGray <: NumberLike
false

julia> AbstractGray <: NumberLike{T} where T
true

julia> AbstractGray{<:FixedPoint} <: NumberLike
true
```
The previous one dispatches on storage type(e.g., N0f8, Float32) instead of colorant type(e.g., RGB, Gray)
Dispatches on storage type is not very useful in practice, so I changed it.

Also rewrite the dispatch test cases
src/ImageCore.jl Outdated Show resolved Hide resolved
@johnnychen94 johnnychen94 changed the title add GenericImage and GenericRGBImage alias add GenericImage and GenericRGBImage alias May 15, 2019
@timholy timholy merged commit 028bb85 into JuliaImages:master May 15, 2019
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

Successfully merging this pull request may close these issues.

2 participants