Added generic function for abstractvectors #188

theogf · 2020-10-26T13:53:54Z

Add supports for vector of vectors by implementing a generic function.
Fixes #152, closes #165, could be extended to #187 (not sure how to dispatch on Iterators in general, but the current implementation would support it)

theogf · 2020-11-03T13:07:08Z

Bump

theogf · 2020-11-16T14:29:24Z

Bump :)

dkarrasch

This looks good to me, except for some minor issues. I should say that this is very hard to tell if it will cause issues with what people may have in their packages, but at least I don't see any.

src/generic.jl

test/test_dists.jl

…ched order of evalutation

theogf · 2020-12-03T10:50:24Z

Thanks for the review, I included the changes you suggested. I am not sure what you meant by switching the loop orders. I tried to be consistent with the existing implementation, but it should not matter in the end

dkarrasch · 2020-12-03T11:21:47Z

I meant to organize the loops in such a way that you write into r in memory-order, so have the 1:j-1 loop before the j+1:end loop, but otherwise keep what is done within the loops.

It seems the org ran out of Travis credits, so CI doesn't work anymore. We need to move to GithubActions...

theogf · 2020-12-03T11:29:57Z

Ok then I made the right change.

It seems the org ran out of Travis credits, so CI doesn't work anymore. We need to move to GithubActions...

I can make a PR for that too :)

dkarrasch · 2020-12-03T11:32:12Z

I can make a PR for that too :)

I think I have something running on my fork. When tests pass there, I'll kick off the PR.

src/generic.jl

dkarrasch · 2020-12-03T12:11:36Z

I think we can do the same loop swap in the matrix version. It's formally the exact same amount of work, but may benefit from better memory access pattern. To the very least, it shouldn't be worse.

theogf · 2020-12-03T13:24:36Z

Tests are failing because of the tests on the AbstractVector{<:Real} as you said. I will just remove these tests for now as I guess it deserves a different PR.

codecov-io · 2020-12-03T14:08:42Z

Codecov Report

Merging #188 (c1b1748) into master (e50899d) will decrease coverage by 0.01%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##           master     #188      +/-   ##
==========================================
- Coverage   95.05%   95.04%   -0.01%     
==========================================
  Files           8        8              
  Lines         688      727      +39     
==========================================
+ Hits          654      691      +37     
- Misses         34       36       +2

Impacted Files	Coverage Δ
src/Distances.jl	`50.00% <0.00%> (-50.00%)`	⬇️
src/metrics.jl	`96.50% <50.00%> (-0.25%)`	⬇️
src/generic.jl	`98.49% <100.00%> (+0.57%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e50899d...c1b1748. Read the comment docs.

dkarrasch · 2020-12-03T14:14:45Z

Awesome! Now let's apply the same trick to the good old matrix-version, and I think this is ready to go. For the Vector{<:Number} tests, I think we need to restrict to a subset of the metrics, and in particular exclude the weighted metrics (or adjust their parameter vector). Maybe by checking something like parameters(dist) === nothing? And the [Sq]Mahalanobis stuff failed.

dkarrasch

I have added scalar tests and the required dims keyword to pairwise. I suggest to replace the three occurrences of transpose in generic.jl by permutedims, since the first acts recursively, but the dims=1 vs dims=2 distinction is only about the orientation of the outermost array.

src/generic.jl

test/test_dists.jl

src/Distances.jl

src/generic.jl

theogf · 2020-12-10T15:41:16Z

@dkarrasch @nalimilan Ok I think all issues are solved now.

dkarrasch

Two small things, and then I think this is ready.

@nalimilan I wonder if it helps FreqTables.jl if we made these pairwise methods fully generic for iterators, and removed the type constraints on a and b. Then the Distances.jl pairwise method would act metric::PreMetric.

dkarrasch · 2020-12-11T14:58:42Z

src/generic.jl

+        for i = (j + 1):n
+            r[i, j] = metric(a[i], a[j])


The compiler may do that on its own, but let's be sure:

Suggested change

for i = (j + 1):n

r[i, j] = metric(a[i], a[j])

aj = a[j]

for i = (j + 1):n

r[i, j] = metric(a[i], aj)

dkarrasch · 2020-12-11T15:00:20Z

src/metrics.jl

@@ -489,6 +489,8 @@ js_divergence(a::AbstractArray, b::AbstractArray) = JSDivergence()(a, b)

 result_type(dist::SpanNormDist, a::AbstractArray, b::AbstractArray) =
     typeof(eval_op(dist, oneunit(eltype(a)), oneunit(eltype(b))))
+result_type(dist::SpanNormDist, a::AbstractArray{<:AbstractArray}, b::AbstractArray{<:AbstractArray}) =
+     typeof(eval_op(dist, oneunit(eltype(first(a))), oneunit(eltype(first(b)))))


Suggested change

typeof(eval_op(dist, oneunit(eltype(first(a))), oneunit(eltype(first(b)))))

typeof(eval_op(dist, oneunit(eltype(eltype(a))), oneunit(eltype(eltype(b)))))

Maybe we should rather override result_type(dist::SpanNormDist, a::Type, b::Type) and have global generic methods that call this? It sounds suboptimal to redefine methods for different inputs types.

nalimilan · 2020-12-11T16:51:24Z

src/generic.jl

@@ -34,7 +34,7 @@ data `a` and `b`.
 """
 result_type(::PreMetric, ::Type, ::Type) = Float64 # fallback
 result_type(dist::PreMetric, a::AbstractArray, b::AbstractArray) = result_type(dist, eltype(a), eltype(b))
-
+result_type(dist::PreMetric, a::AbstractArray{<:AbstractArray}, b::AbstractArray{<:AbstractArray}) = result_type(dist, eltype(eltype(a)), eltype(eltype(b)))


Suggested change

result_type(dist::PreMetric, a::AbstractArray{<:AbstractArray}, b::AbstractArray{<:AbstractArray}) = result_type(dist, eltype(eltype(a)), eltype(eltype(b)))

result_type(dist::PreMetric, a::AbstractArray{<:AbstractArray}, b::AbstractArray{<:AbstractArray}) =

result_type(dist, eltype(eltype(a)), eltype(eltype(b)))

nalimilan · 2020-12-11T16:54:09Z

src/generic.jl

+    pairwise!(r::AbstractMatrix, metric::PreMetric,
+              a::AbstractVector, b::AbstractVector=a)


The docstring should explain what happens for vectors. It should probably even be a separate docstring as there's almost nothing in common in the description.

nalimilan · 2020-12-11T16:55:49Z

src/metrics.jl

@@ -489,6 +489,8 @@ js_divergence(a::AbstractArray, b::AbstractArray) = JSDivergence()(a, b)

 result_type(dist::SpanNormDist, a::AbstractArray, b::AbstractArray) =
     typeof(eval_op(dist, oneunit(eltype(a)), oneunit(eltype(b))))
+result_type(dist::SpanNormDist, a::AbstractArray{<:AbstractArray}, b::AbstractArray{<:AbstractArray}) =
+     typeof(eval_op(dist, oneunit(eltype(first(a))), oneunit(eltype(first(b)))))


Maybe we should rather override result_type(dist::SpanNormDist, a::Type, b::Type) and have global generic methods that call this? It sounds suboptimal to redefine methods for different inputs types.

dkarrasch · 2020-12-12T14:56:28Z

The more I think about it, the more I believe this really is not about AbstractVector methods, but about iterable objects that contain data points between which we want to compute distances. I included iterator-based pairwise and colwise methods now in #194. My result_type implementation generically handles arrays of arrays (or arrays), but avoids things like #188 (comment). I guess it kind of supersedes this PR (I also included that scalar pairwise tests).

theogf added 4 commits October 26, 2020 14:51

Added generic function for abstractvectors

16e6e42

Solving ambiguities

f43ceaa

Fixed docs and error

cbec7d1

Replaced eachcol

266d521

dkarrasch reviewed Dec 2, 2020

View reviewed changes

src/generic.jl Show resolved Hide resolved

src/generic.jl Show resolved Hide resolved

src/generic.jl Outdated Show resolved Hide resolved

test/test_dists.jl Show resolved Hide resolved

theogf added 2 commits December 3, 2020 11:48

Included suggested changes: using require_one_based_indexing and swit…

2ac3b3a

…ched order of evalutation

added tests for vector of reals

11e402b

dkarrasch reviewed Dec 3, 2020

View reviewed changes

src/generic.jl Outdated Show resolved Hide resolved

theogf added 3 commits December 3, 2020 14:15

Merge remote-tracking branch 'JuliaStats/master' into vector_support

706dd4e

Replaced badge to README

5fbe39e

Switched back the order of evaluations

51568e0

theogf added 2 commits December 3, 2020 14:25

Removed tests on abstractvector{<:Real}

4595fc0

swapped block order

76caa1f

Swapping blocks for matrices as well

c1b1748

dkarrasch reviewed Dec 4, 2020

View reviewed changes

src/generic.jl Show resolved Hide resolved

src/generic.jl Show resolved Hide resolved

src/generic.jl Outdated Show resolved Hide resolved

test/test_dists.jl Show resolved Hide resolved

dkarrasch mentioned this pull request Dec 5, 2020

Allow generators and iterators #194

Merged

nalimilan reviewed Dec 7, 2020

View reviewed changes

nalimilan mentioned this pull request Dec 8, 2020

Add pairwise nalimilan/FreqTables.jl#54

Draft

theogf added 2 commits December 10, 2020 13:55

Added space at the end and added tests for scalar metrics

21e8911

Fixed unconsistent use of dims and corrected alignment

1d5c740

theogf added 6 commits December 10, 2020 14:00

Style require_one_based_indexing

9705448

Adapted result_type to be less restrictive

65787b9

Solved duplicate functions

b086a9b

Fixed ambiguity?

0ca4d47

Second fix

891cf8c

Switching back to second argument = a (SemiMetric vs PreMetric)

5c296ba

dkarrasch reviewed Dec 11, 2020

View reviewed changes

nalimilan reviewed Dec 11, 2020

View reviewed changes

dkarrasch closed this in #194 Dec 18, 2020

theogf deleted the vector_support branch December 18, 2020 11:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added generic function for abstractvectors #188

Added generic function for abstractvectors #188

theogf commented Oct 26, 2020 •

edited by dkarrasch

Loading

theogf commented Nov 3, 2020

theogf commented Nov 16, 2020

dkarrasch left a comment

theogf commented Dec 3, 2020

dkarrasch commented Dec 3, 2020

theogf commented Dec 3, 2020

dkarrasch commented Dec 3, 2020

dkarrasch commented Dec 3, 2020

theogf commented Dec 3, 2020

codecov-io commented Dec 3, 2020 •

edited

Loading

dkarrasch commented Dec 3, 2020

dkarrasch left a comment

theogf commented Dec 10, 2020

dkarrasch left a comment

dkarrasch Dec 11, 2020

dkarrasch Dec 11, 2020

nalimilan Dec 11, 2020

nalimilan Dec 11, 2020

nalimilan Dec 11, 2020

nalimilan Dec 11, 2020

dkarrasch commented Dec 12, 2020

	typeof(eval_op(dist, oneunit(eltype(first(a))), oneunit(eltype(first(b)))))
	typeof(eval_op(dist, oneunit(eltype(eltype(a))), oneunit(eltype(eltype(b)))))

	result_type(dist::PreMetric, a::AbstractArray{<:AbstractArray}, b::AbstractArray{<:AbstractArray}) = result_type(dist, eltype(eltype(a)), eltype(eltype(b)))
	result_type(dist::PreMetric, a::AbstractArray{<:AbstractArray}, b::AbstractArray{<:AbstractArray}) =
	result_type(dist, eltype(eltype(a)), eltype(eltype(b)))

		pairwise!(r::AbstractMatrix, metric::PreMetric,
		a::AbstractVector, b::AbstractVector=a)

Added generic function for abstractvectors #188

Added generic function for abstractvectors #188

Conversation

theogf commented Oct 26, 2020 • edited by dkarrasch Loading

theogf commented Nov 3, 2020

theogf commented Nov 16, 2020

dkarrasch left a comment

Choose a reason for hiding this comment

theogf commented Dec 3, 2020

dkarrasch commented Dec 3, 2020

theogf commented Dec 3, 2020

dkarrasch commented Dec 3, 2020

dkarrasch commented Dec 3, 2020

theogf commented Dec 3, 2020

codecov-io commented Dec 3, 2020 • edited Loading

Codecov Report

dkarrasch commented Dec 3, 2020

dkarrasch left a comment

Choose a reason for hiding this comment

theogf commented Dec 10, 2020

dkarrasch left a comment

Choose a reason for hiding this comment

dkarrasch Dec 11, 2020

Choose a reason for hiding this comment

dkarrasch Dec 11, 2020

Choose a reason for hiding this comment

nalimilan Dec 11, 2020

Choose a reason for hiding this comment

nalimilan Dec 11, 2020

Choose a reason for hiding this comment

nalimilan Dec 11, 2020

Choose a reason for hiding this comment

nalimilan Dec 11, 2020

Choose a reason for hiding this comment

dkarrasch commented Dec 12, 2020

theogf commented Oct 26, 2020 •

edited by dkarrasch

Loading

codecov-io commented Dec 3, 2020 •

edited

Loading