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

rework result_type #140

Merged
merged 3 commits into from Aug 15, 2019
Merged

rework result_type #140

merged 3 commits into from Aug 15, 2019

Conversation

johnnychen94
Copy link
Contributor

@johnnychen94 johnnychen94 commented Aug 8, 2019

The essence of result_type is based on the eltype(a), and result_type(::PreMetric, ::AbstractArray, ::AbstractArray) only serves as an convenient method.

There are no functionality changes in this commit, only to reorganize the codes to ease future development.


For example, in ImageDistances.jl, we can easily extend result_type to Colorant types if we dispatch on Type:

for (ATa, ATb) in ((AbstractGray, AbstractGray),
                   (AbstractGray, Number      ),
                   (Number      , AbstractGray),
                   (PromoteType , PromoteType )) # Union{FixedPoint, Bool}
    @eval function result_type(dist::PreMetric, a::Type{Ta}, b::Type{Tb}) where {Ta<:$ATa, Tb<:$ATb}
        T1 = eltype(floattype(Ta)) # Gray{N0f8} -> Float32
        T2 = eltype(floattype(Tb))
        result_type(dist, T1, T2)
    end
end

In the meantime, dispatching on AbstractArray would introduces a lot of method ambiguities.

The essence of `result_type` is based on the `eltype(a)`, and
`result_type(::PreMetric, ::AbstractArray, ::AbstractArray)` only
serves as an convenient method.

There's no functionality changes in this commit, only to reorganize
the codes to ease future development.
Copy link
Member

@nalimilan nalimilan left a comment

Choose a reason for hiding this comment

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

Sure, why not. Couldn't we even find a generic definition using oneunit and float?

src/generic.jl Outdated
```julia-repl
julia> T = result_type(Euclidean(), Float32, Float64)
Float64
julia> r = zeros(T, 2, 2)
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look relevant here.

Copy link
Member

Choose a reason for hiding this comment

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

I mean that this is a docstring for an internal function, so it doesn't seem necessary to explain what can be the use of that function. Anyway, if you prefer showing an example calling pairwise, please turn it into a runnable doctest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I simply delete this part in the new commit; the only reason I added this seeming-too-obvious example is because result_type is exported.

Copy link
Member

Choose a reason for hiding this comment

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

I hadn't realized it was exported. Anyway, looks good that way.

src/generic.jl Outdated Show resolved Hide resolved
@johnnychen94
Copy link
Contributor Author

johnnychen94 commented Aug 9, 2019

Couldn't we even find a generic definition using oneunit and float?

I tried, but then I realized it's better not to change the behavior of those algorithms in the first PR. You're suggesting some breaking changes though.

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

3 participants