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

move storage type conversion trait from ImageCore to Colors #475

Open
johnnychen94 opened this issue May 8, 2021 · 6 comments
Open

move storage type conversion trait from ImageCore to Colors #475

johnnychen94 opened this issue May 8, 2021 · 6 comments

Comments

@johnnychen94
Copy link
Member

It looks like the most appropriate place for these traits is here:

https://github.com/JuliaImages/ImageCore.jl/blob/4fb132187d68054be7413ec229071115c14f72bd/src/convert_reinterpret.jl#L61-L107

@kimikage
Copy link
Collaborator

kimikage commented May 9, 2021

I have no objection to "defining" them in Colors or ColorTypes, but I am not comfortable exporting them. 😕

I can fully understand why you would prefer to have them defined in Colors (or ColorTypes), but I don't see what the trouble is with having them in ImageCore.

The same goes for clamp01 and clamp01nan.(cf. #469 (comment))

@kimikage
Copy link
Collaborator

kimikage commented May 9, 2021

Also, as you know, there is the inconsistency problem with float other than for FixedPoint/AbstractFloat.

@johnnychen94
Copy link
Member Author

johnnychen94 commented May 9, 2021

I have no objection to "defining" them in Colors or ColorTypes, but I am not comfortable exporting them.

I'm okay with this since it's mainly the package developer that needs to take care of the storage type.

My main consideration when proposing this move is that I want to give better support for them on broader color types. Currently,

julia> n0f8(RGB)
RGB{N0f8}

julia> n0f8(RGB{Float32})
RGB{N0f8}

julia> n0f8(RGB24)
ERROR: TypeError: in Type{...} expression, expected UnionAll, got Type{RGB24}
Stacktrace:
 [1] n0f8(#unused#::Type{RGB24})
   @ ImageCore ~/.julia/packages/ImageCore/iXG0W/src/convert_reinterpret.jl:82
 [2] top-level scope
   @ REPL[21]:1

This need arises when I tried to convert all colorant images to N0f8/UInt8 byte sequences:

canonical_colorant_type(::Type{CT}) where CT<:Colorant = n0f8(CT)
canonical_colorant_type(::Type{T}) where T<:Real = Gray{N0f8}

but I don't see what the trouble is with having them in ImageCore.

The only reason that I feel it better to live in Colors is that they are pixel-level operations. 😄

@kimikage
Copy link
Collaborator

kimikage commented May 9, 2021

Even if we do the migration, I don't see any reason not to fix n0f8(RGB24) etc. in the current ImageCore. (I'm not sure whether n0f8(RGB24) should return RGB{N0f8} or RGB24, though.)

@kimikage
Copy link
Collaborator

I think it's straightforward to define n0f8 and so on in FixedPointNumbers and implement n0f8(::Colorant) and so on in ColorTypes. However, I'm not sure that it's really a good idea for FixedPointNumbers to provide such non-generic functions.
Also, I don't think float32 and float64 are functions that should be exported by the low-level packages.

However, I think the following should be solved within ColorTypes.

julia> convert(Colorant{Float32}, RGB24(1))
ERROR: nonparametric type RGB24 has ambiguous destination Colorant{Float32, N} where N

(Of course, this is a known issue:
https://github.com/JuliaGraphics/ColorTypes.jl/blob/81297fff41e28dc64ace72a37c090ee22363ba71/test/conversions.jl#L446)

@johnnychen94
Copy link
Member Author

Makes sense to me; either move or not are good. We can keep this issue open until we decide the 1.0 status.

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