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

Use of_eltype instead of convert lambda #95

Merged
merged 2 commits into from
Aug 9, 2021

Conversation

barucden
Copy link
Collaborator

@barucden barucden commented Aug 9, 2021

As suggested by @timholy in #17, this PR changes mappedarray(c->convert(T,c), img) to of_eltype(T, img). It results
in MappedArray instead of ReadonlyMappedArray, and it is more descriptive too.

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

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}"

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}"
```
Copy link
Collaborator

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a test case for ConvertEltype(Gray, img)? If not we might want to add one and ensure it passes the @inferred and eltype(...) <: Gray test.

I'm so glad that you're pulling this package off!

Copy link
Collaborator

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assume the test passes.

@barucden
Copy link
Collaborator Author

barucden commented Aug 9, 2021

Tests passed. Thanks!

@barucden barucden merged commit b3a1873 into Evizero:master Aug 9, 2021
@barucden barucden deleted the of_eltype branch August 9, 2021 15:03
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.

None yet

2 participants