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 eachindex and axes in loops #132

Merged
merged 4 commits into from
May 18, 2022
Merged

use eachindex and axes in loops #132

merged 4 commits into from
May 18, 2022

Conversation

heliosdrm
Copy link
Collaborator

Replace syntax for i in 1:length(x) by for i in eachindex(x), and for i in 1:size(x,d) by for i in axes(x,d), as recommended.

@Datseris
Copy link
Member

As recommended by whom and why? :D I don't think it makes much of a difference but sure.

What I've noticed, that most likely will make a lot of difference, is that we do


    for j in eachindex(y), i in eachindex(x)
        @inbounds d[i,j] = evaluate(metric, x[i], y[j])
    end

instead of using any of the pairwise, rowwise, colwise from Distances.jl, which are much more performant versions that use BLAS and SIMD acceleration to compute distances across many pairs of vectors.

@heliosdrm
Copy link
Collaborator Author

The recommendation is here: https://julialang.org/blog/2016/02/iteration/

Also widely commented in this recent and very active post: https://discourse.julialang.org/t/discussion-on-why-i-no-longer-recommend-julia-by-yuri-vishnevsky/81151

That recommendation is done for the sake of generality; I don't think that the problems discussed there really apply in the code of this package. But I thought that if there is a consensus of for i in eachindex(x) being better Julia style than for i in 1:length(x), it makes no harm to do this little change and follow that style.

On the other hand, I don't understand this commentary:

What I've noticed, that most likely will make a lot of difference, is that we do


    for j in eachindex(y), i in eachindex(x)
        @inbounds d[i,j] = evaluate(metric, x[i], y[j])
    end

instead of using any of the pairwise, rowwise, colwise from Distances.jl, which are much more performant versions that use BLAS and SIMD acceleration to compute distances across many pairs of vectors.

No big change was proposed there, only replacing of ranges by eachindex. Do you mean that there are further changes that should be done?

@Datseris Datseris merged commit 135ae13 into master May 18, 2022
@Datseris Datseris deleted the clean_loop_syntax branch May 18, 2022 15:30
@Datseris
Copy link
Member

I didn't know that this was an established consensus. Now that I do, I will try to follow it. Sure, this is a straight forward stylistic improvement.

On the other hand, I don't understand this commentary

yeah, that was not really relevant with this PR, but I just saw it because it modified this source code. The short story is that Distances.jl recommends using pairwise when wanting to compute pairwise distances across collections of points. We don't do that here, and I believe because of that we miss some BLAS+SIMD acceleration that Distances.jl offers under the hood for some standard distances like Euclidean. It is a separate topic of course, so needs to be done in a different PR, but should definitely help with #130

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.

2 participants