move storage type conversion trait from ImageCore to Colors #475

Activity

1. kimikage commented on May 9, 2021

   

   kimikage

   on May 9, 2021

   Last edited by kimikage

   Collaborator

   More actions

   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))
2. kimikage commented on May 9, 2021

   

   kimikage

   on May 9, 2021

   Last edited by kimikage

   Collaborator

   More actions

   Also, as you know, there is the inconsistency problem with float other than for FixedPoint/AbstractFloat.
3. johnnychen94 commented on May 9, 2021

   

   johnnychen94

   on May 9, 2021

   Last edited by johnnychen94

   MemberAuthor

   More actions

   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. 😄
4. kimikage commented on May 9, 2021

   

   kimikage

   on May 9, 2021

   Collaborator

   More actions

   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.)

   👍React with 👍1Reacted by Johnny Chen
5. kimikage commented on May 15, 2021

   

   kimikage

   on May 15, 2021

   Collaborator

   More actions

   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)

   👍React with 👍1Reacted by Johnny Chen
6. johnnychen94 commented on May 15, 2021

   

   johnnychen94

   on May 15, 2021

   MemberAuthor

   More actions

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

   👍React with 👍1Reacted by kimikage