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

ConvertEltype(Gray) not working as expect anymore #17

Closed
Evizero opened this issue Mar 30, 2018 · 5 comments
Closed

ConvertEltype(Gray) not working as expect anymore #17

Evizero opened this issue Mar 30, 2018 · 5 comments
Labels
Milestone

Comments

@Evizero
Copy link
Owner

Evizero commented Mar 30, 2018

It seems that for some reason or another, leaving the inner type open for Gray now causes fatal issues with inference. Will either have to force user to be specific with ConvertEltype(Gray{INNER}) or see if I can fix this.

@Evizero
Copy link
Owner Author

Evizero commented Apr 8, 2018

Seems to only be an issue if followed by a SplitChannel(). I suspect the issue is somewhere upstream with channelview

@johnnychen94 johnnychen94 added this to the v0.6 milestone Dec 19, 2019
@johnnychen94
Copy link
Collaborator

johnnychen94 commented Dec 19, 2019

function applyeager(op::ConvertEltype{T}, img::AbstractArray, param) where T
maybe_copy(convert(AbstractArray{T}, img))
end
function applylazy(op::ConvertEltype{T}, img::AbstractArray, param) where T
mappedarray(c->convert(T,c), img)
end

A simple decision could be: if storage type T here is Any, then use eltype(eltype(img)) as the output type. Of course we need to deal with cases for color types that don't support N0f8 or Bool (e.g., Lab)

Update:

This would cause type-instability so it doesn't work.

@johnnychen94
Copy link
Collaborator

causes fatal issues with inference

I guess it's no longer an issue now. @Evizero does this look good to you?

julia> @inferred(augment(img, ConvertEltype(Gray))) |> summary # Gray{Any}
"300×400 Array{Gray{Any},2} with eltype Gray"

julia> pl = SplitChannels() |> ConvertEltype(Gray)
2-step Augmentor.ImmutablePipeline:
 1.) Split colorant into its color channels
 2.) Convert eltype to Gray{Any}

julia> @inferred(augment(img, pl)) |> summary # Gray{N0f8}
"4×300×400 Array{Gray{N0f8},3} with eltype Gray{Normed{UInt8,8}}"

@timholy
Copy link

timholy commented Feb 25, 2020

Just a brief comment, instead of mappedarray(c->convert(T,c), img) it's slightly better to use of_eltype(T, img) because it also defines the inverse operation and thus supports setindex!. https://github.com/JuliaArrays/MappedArrays.jl#of_eltype

barucden added a commit to barucden/Augmentor.jl that referenced this issue Aug 9, 2021
As suggested by @timholy in Evizero#17, this commit changes
`mappedarray(c->convert(T,c), img)` to `of_eltype(T, img)`. It results
into `MappedArray` instead of `ReadonlyMappedArray`, and it is more
descriptive too.

This commit does not deal with Evizero#17, but I can confirm that the issue
seems to be fixed already (which @johnnychen94 noticed before) and can
be closed:

```julia
julia> using Augmentor, Test

julia> img = testpattern();

julia> @inferred(augment(img, ConvertEltype(Gray))) |> summary # Gray{Any}
"300×400 Array{Gray,2} with eltype Gray"

julia> pl = SplitChannels() |> ConvertEltype(Gray)
2-step Augmentor.ImmutablePipeline:
 1.) Split colorant into its color channels
 2.) Convert eltype to Gray

julia> @inferred(augment(img, pl)) |> summary # Gray{N0f8}
"4×300×400 Array{Gray{N0f8},3} with eltype Gray{FixedPointNumbers.N0f8}"
```
barucden added a commit to barucden/Augmentor.jl that referenced this issue Aug 9, 2021
As suggested by @timholy in Evizero#17, this commit changes
`mappedarray(c->convert(T,c), img)` to `of_eltype(T, img)`. It results
into `MappedArray` instead of `ReadonlyMappedArray`, and it is more
descriptive too.

This commit does not deal with Evizero#17, but I can confirm that the issue
seems to be fixed already (which @johnnychen94 noticed before) and can
be closed:

```julia
julia> using Augmentor, Test

julia> img = testpattern();

julia> @inferred(augment(img, ConvertEltype(Gray))) |> summary # Gray{Any}
"300×400 Array{Gray,2} with eltype Gray"

julia> pl = SplitChannels() |> ConvertEltype(Gray)
2-step Augmentor.ImmutablePipeline:
 1.) Split colorant into its color channels
 2.) Convert eltype to Gray

julia> @inferred(augment(img, pl)) |> summary # Gray{N0f8}
"4×300×400 Array{Gray{N0f8},3} with eltype Gray{FixedPointNumbers.N0f8}"
```
barucden added a commit to barucden/Augmentor.jl that referenced this issue Aug 9, 2021
As suggested by @timholy in Evizero#17, this commit changes
`mappedarray(c->convert(T,c), img)` to `of_eltype(T, img)`. It results
into `MappedArray` instead of `ReadonlyMappedArray`, and it is more
descriptive too.

This commit does not deal with Evizero#17, but I can confirm that the issue
seems to be fixed already (which @johnnychen94 noticed before) and can
be closed:

```julia
julia> using Augmentor, Test

julia> img = testpattern();

julia> @inferred(augment(img, ConvertEltype(Gray))) |> summary # Gray{Any}
"300×400 Array{Gray,2} with eltype Gray"

julia> pl = SplitChannels() |> ConvertEltype(Gray)
2-step Augmentor.ImmutablePipeline:
 1.) Split colorant into its color channels
 2.) Convert eltype to Gray

julia> @inferred(augment(img, pl)) |> summary # Gray{N0f8}
"4×300×400 Array{Gray{N0f8},3} with eltype Gray{FixedPointNumbers.N0f8}"
```
@barucden
Copy link
Collaborator

barucden commented Aug 9, 2021

I confirm that the issue is gone. See #95, which implements a few test cases for this.

@barucden barucden closed this as completed Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants