search

JuliaImages / ImageDistances.jl

Distances between N-dimensional images
https://github.com/JuliaImages/Images.jl
Other
15 stars 8 forks source link

incorrect/inconsistent definition of colwise #13

Closed johnnychen94 closed 5 years ago

johnnychen94 commented 5 years ago 
# Distances
julia> colwise(Euclidean(), rand(3,1),rand(3,1))
1-element Array{Float64,1}:
 0.6134618205636844

julia> colwise(Euclidean(), rand(1,3),rand(1,3))
3-element Array{Float64,1}:
 0.1528773435016244 
 0.30766429246370963
 0.07516318957681967
# ImageDistances
julia> struct MyImgDist <: ImageMetric end
julia> ImageDistances.evaluate(d::MyImgDist, imgA, imgB) = π
julia> imgs = [ones(3,3), ones(3,3), zeros(3,3)];
julia> size(imgs)
(3,)

julia> colwise(MyImgDist(), imgs, imgs)
3-element Array{Irrational{:π},1}:
 π = 3.1415926535897...
 π = 3.1415926535897...
 π = 3.1415926535897...

I think for image types we only need to support two kinds of input:

where the colwise distance degenerate to situation that column length 1. Column length != 1 has no direct meaning.

Since this package is still under development, can I directly correct the behavior of this function without deprecation? @timholy

juliohm commented 5 years ago

Hi @johnnychen94 , I am back home after a business trip. Could you please clarify the issue with the current implementation? The output in the example you wrote seems correct to me. It should return a list of 3 distances.

johnnychen94 commented 5 years ago

Nice to hear you back Júlio

The current colwise function in Distances doesn’t support all-vector inputs. It should be one of the following:

Vector A and Vector B would raise an MethodError.

The current colwise function in ImageDistances is inconsistent in this sense.

Best,

Johnny

juliohm commented 5 years ago

I see. You want to make the signature in ImageDistances.jl match with that in Distances.jl. I think the idea of colwise in ImageDistances.jl is to compute a distance between two lists of objects. Strictly speaking it could be renamed to listwise or iterablewise. What is your proposed change? What is the use case you have in mind that may cause problems?

On Sat, Apr 27, 2019, 08:49 Johnny Chen notifications@github.com wrote:

Nice to hear you back Júlio

The current colwise function in Distances doesn’t support all-vector inputs. It should be one of the following:

  • mn Matrix A and mn Matrix B
  • m*n Matrix A and length-m Vector B (broadcasts)
  • length-vector A and m*n Matrix B

Vector A and Vector B would raise an MethodError.

The current colwise function in ImageDistances is inconsistent in this sense.

Best,

Johnny 2019年4月27日 +0800 19:39 Júlio Hoffimann notifications@github.com，写道：

Hi @johnnychen94< https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fjohnnychen94&data=02%7C01%7C%7Cf4e4387e49d944eaf5fa08d6cb04f944%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636919619549176029&sdata=FPTbd93MeGMsIbv0qeQ7ImN20ro7iCJt6coHJTPk7dI%3D&reserved=0> , I am back home after a business trip. Could you please clarify the issue with the current implementation? The output in the example you wrote seems correct to me. It should return a list of 3 distances.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub< https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FJuliaImages%2FImageDistances.jl%2Fissues%2F13%23issuecomment-487278749&data=02%7C01%7C%7Cf4e4387e49d944eaf5fa08d6cb04f944%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636919619549186034&sdata=wlO5dtA18WDJJth%2Fzs9kKAdrlTdC2x7RwIs10JvQnPQ%3D&reserved=0>, or mute the thread< https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FACCIGQYJBBXJ7DX7HNEIL63PSQ3OFANCNFSM4HIU7W4A&data=02%7C01%7C%7Cf4e4387e49d944eaf5fa08d6cb04f944%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636919619549196045&sdata=EGuO3WJYUUFl639NOG9AMQB0aIF7SMDQGl1QzWM8%2FfA%3D&reserved=0>.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/JuliaImages/ImageDistances.jl/issues/13#issuecomment-487279408, or mute the thread https://github.com/notifications/unsubscribe-auth/AAZQW3NIJJPKAVRHRL5QBXTPSQ4TRANCNFSM4HIU7W4A .

johnnychen94 commented 5 years ago

@juliohm

With “eltype(imgsA)==AbstractArray” (Not Number in Distances case)

My idea is to still keep all-vector input valid as it is now since it makes sense(at least to me and you)

In the meantime, support 1xN Matrix inputs where num_column is exactly 1. This’s also a colwise distance:)

For MxN Matrix inputs, I planned to make it invalid since generally speaking it’s not very meaningful.

How do you like it?

juliohm commented 5 years ago

You mean that colwise should also handle the case where both lists contain only 1 object ? colwise(dist, imgA, imgB)? If that is the case I think the modification is not needed since we already have evaluate. Please let me know it is makes sense to you.

On Sat, Apr 27, 2019, 13:28 Johnny Chen notifications@github.com wrote:

@juliohm https://github.com/juliohm

With “eltype(imgsA)==AbstractArray” (Not Number in Distances case)

My idea is to still keep all-vector input valid as it is now since it makes sense(at least to me and you)

In the meantime, support 1xN Matrix inputs where num_column is exactly 1. This’s also a colwise distance:)

For MxN Matrix inputs, I planned to make it invalid since generally speaking it’s not very meaningful.

How do you like it?

Sent from mini-github https://github.com/kezhenxu94/mini-github

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JuliaImages/ImageDistances.jl/issues/13#issuecomment-487299922, or mute the thread https://github.com/notifications/unsubscribe-auth/AAZQW3MSYRPR2N5G4DRHASTPSR5LFANCNFSM4HIU7W4A .

johnnychen94 commented 5 years ago

imgsA::AbstractArray{<:AbstractArray}

(1,n) means num_column=n and num_row=1, in this case colwise degrades to iteraterwise. (each column has exactly one image)

Johnny

juliohm commented 5 years ago

I see. You mean the current code is allowing (n,1) arrays and it shouldn't. Please consider fixing the signature as suggested. A PR would be great to assess the changes in the code.

On Sat, Apr 27, 2019, 14:09 Johnny Chen notifications@github.com wrote:

imgsA::AbstractArray{<:AbstractArray}

  • valid inputs: size(imgsA) == (1, n) or size(imgsA) == (n,)
  • Invalid inputs: size(imgsA) == (n,1)

(1,n) means num_column=n and num_row=1, in this case colwise degrades to iteraterwise. (each column has exactly one image)

Johnny 2019年4月28日 +0800 00:57 Júlio Hoffimann ，写道： You mean that colwise should also handle the case where both lists contain only 1 object ? colwise(dist, imgA, imgB)? If that is the case I think the modification is not needed since we already have evaluate. Please let me know it is makes sense to you.

On Sat, Apr 27, 2019, 13:28 Johnny Chen notifications@github.com wrote:

@juliohm https://github.com/juliohm

With “eltype(imgsA)==AbstractArray” (Not Number in Distances case)

My idea is to still keep all-vector input valid as it is now since it makes sense(at least to me and you)

In the meantime, support 1xN Matrix inputs where num_column is exactly 1. This’s also a colwise distance:)

For MxN Matrix inputs, I planned to make it invalid since generally speaking it’s not very meaningful.

How do you like it?

Sent from mini-github https://github.com/kezhenxu94/mini-github

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub < https://github.com/JuliaImages/ImageDistances.jl/issues/13#issuecomment-487299922>,

or mute the thread < https://github.com/notifications/unsubscribe-auth/AAZQW3MSYRPR2N5G4DRHASTPSR5LFANCNFSM4HIU7W4A>

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub< https://nam01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FJuliaImages%2FImageDistances.jl%2Fissues%2F13%23issuecomment-487302353&data=02%7C01%7C%7C0104efca047944f5f14708d6cb31781e%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636919810655761346&sdata=55I1iwprffJdKJvksK8alPgqAqch7JEiu8wTmpTKENI%3D&reserved=0>, or mute the thread< https://nam01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FACCIGQ3I6PLYOIRN6P7GVADPSSAYRANCNFSM4HIU7W4A&data=02%7C01%7C%7C0104efca047944f5f14708d6cb31781e%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636919810655771357&sdata=Kg0Z0ZKsh%2B%2B3TIgiH0qIK3AVmf5zJLazyCVLQ0Q4pR8%3D&reserved=0>.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JuliaImages/ImageDistances.jl/issues/13#issuecomment-487303432, or mute the thread https://github.com/notifications/unsubscribe-auth/AAZQW3JJI4KBSSSR4CPDRRLPSSCETANCNFSM4HIU7W4A .