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

Sorting across different datatypes #17

Merged
merged 3 commits into from
Sep 13, 2023
Merged

Conversation

putianyi889
Copy link
Contributor

@putianyi889 putianyi889 commented Sep 13, 2023

Before this PR:

julia> a,b,c = 2,1,3
(2, 1, 3)

julia> @btime TupleTools.sort((a,b,c))
  232.982 ns (2 allocations: 64 bytes)
(1, 2, 3)

julia> TupleTools.sort((2,1.0))
ERROR: MethodError: no method matching _split(::Tuple{Int64, Float64})

Closest candidates are:
  _split(::Tuple{Vararg{T, N}} where T) where N
   @ TupleTools C:\Users\pty\.julia\packages\TupleTools\ecrzx\src\TupleTools.jl:253

Stacktrace:
 [1] _sort
   @ C:\Users\pty\.julia\packages\TupleTools\ecrzx\src\TupleTools.jl:245 [inlined]
 [2] sort(t::Tuple{Int64, Float64}; lt::Function, by::Function, rev::Bool)
   @ TupleTools C:\Users\pty\.julia\packages\TupleTools\ecrzx\src\TupleTools.jl:243
 [3] sort(t::Tuple{Int64, Float64})
   @ TupleTools C:\Users\pty\.julia\packages\TupleTools\ecrzx\src\TupleTools.jl:243
 [4] top-level scope
   @ REPL[40]:1

After this PR:

julia> @btime TupleTools.sort((a,b,c))
  247.842 ns (2 allocations: 64 bytes)
(1, 2, 3)

julia> c = 3.0
3.0

julia> @btime TupleTools.sort((a,b,c))
  752.857 ns (5 allocations: 128 bytes)
(1, 2, 3.0)

julia> @btime TupleTools.sort((2.0,1.0,c))
  258.457 ns (2 allocations: 64 bytes)
(1.0, 2.0, 3.0)

@Jutho
Copy link
Owner

Jutho commented Sep 13, 2023

How does this behave with respect to inference? I guess it is stil fine for the homogeneous case, but I assume the output type of the inhomogeneous case cannot be inferred?

@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Merging #17 (3a7d62b) into master (2eaf240) will increase coverage by 0.65%.
Report is 2 commits behind head on master.
The diff coverage is 100.00%.

❗ Current head 3a7d62b differs from pull request most recent head 20333f5. Consider uploading reports for the commit 20333f5 to get more accurate results

@@            Coverage Diff             @@
##           master      #17      +/-   ##
==========================================
+ Coverage   87.73%   88.39%   +0.65%     
==========================================
  Files           1        1              
  Lines         106      112       +6     
==========================================
+ Hits           93       99       +6     
  Misses         13       13              
Files Changed Coverage Δ
src/TupleTools.jl 88.39% <100.00%> (+0.65%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@putianyi889
Copy link
Contributor Author

I've updated the tests. The inhomogeneous case does cost significantly more.

@Jutho
Copy link
Owner

Jutho commented Sep 13, 2023

It is indeed nontrivial to benchmark these methods, without the compiler not completely expanding the operation. The current benchmarks are also not the proper way of doing it, as you now rely on global variables, leading to allocations even in the homogeneous case (which should not be there).

@putianyi889
Copy link
Contributor Author

It is indeed nontrivial to benchmark these methods, without the compiler not completely expanding the operation. The current benchmarks are also not the proper way of doing it, as you now rely on global variables, leading to allocations even in the homogeneous case (which should not be there).

Do you have an idea how to benchmark properly?

@putianyi889
Copy link
Contributor Author

For type inference

julia> @code_warntype TupleTools.sort((a,b,c))
MethodInstance for TupleTools.sort(::Tuple{Int64, Int64, Float64})
  from sort(t::Tuple; lt, by, rev) @ TupleTools C:\Users\pty\.julia\dev\TupleTools\src\TupleTools.jl:243
Arguments
  #self#::Core.Const(TupleTools.sort)
  t::Tuple{Int64, Int64, Float64}
Body::Union{Tuple{Float64, Int64, Int64}, Tuple{Int64, Float64, Int64}, Tuple{Int64, Int64, Float64}}
1%1 = TupleTools.:(var"#sort#3")(TupleTools.isless, TupleTools.identity, false, #self#, t)::Union{Tuple{Float64, Int64, Int64}, Tuple{Int64, Float64, Int64}, Tuple{Int64, Int64, Float64}}
└──      return %1


julia> @code_warntype TupleTools.sort((float(a),float(b),c))
MethodInstance for TupleTools.sort(::Tuple{Float64, Float64, Float64})
  from sort(t::Tuple; lt, by, rev) @ TupleTools C:\Users\pty\.julia\dev\TupleTools\src\TupleTools.jl:243
Arguments
  #self#::Core.Const(TupleTools.sort)
  t::Tuple{Float64, Float64, Float64}
Body::Tuple{Float64, Float64, Float64}
1%1 = TupleTools.:(var"#sort#3")(TupleTools.isless, TupleTools.identity, false, #self#, t)::Tuple{Float64, Float64, Float64}
└──      return %1

@Jutho
Copy link
Owner

Jutho commented Sep 13, 2023

It also took me a while to figure out, but I think this is one (probably there are other) correct way:

julia> function f(t)
           @btime TupleTools.sort($t)
       end
f (generic function with 1 method)

julia> f((1,2,3))
  3.030 ns (0 allocations: 0 bytes)
(1, 2, 3)

julia> f((1,2,3,4))
  4.924 ns (0 allocations: 0 bytes)
(1, 2, 3, 4)

julia> f((1,2,3,4,5,6,7,8))
  14.466 ns (0 allocations: 0 bytes)
(1, 2, 3, 4, 5, 6, 7, 8)

@putianyi889
Copy link
Contributor Author

putianyi889 commented Sep 13, 2023

seems like these are the same

julia> d=(a,b,c)
(2, 1, 3.0)

julia> @btime TupleTools.sort(d)
  484.118 ns (4 allocations: 96 bytes)
(1, 2, 3.0)

julia> f((2,1,3.0))
  420.930 ns (4 allocations: 96 bytes)
(1, 2, 3.0)

Edit: no. your attempt seems more correct

julia> e=(float(a),float(b),float(c))
(2.0, 1.0, 3.0)

julia> @btime TupleTools.sort(e)
  87.097 ns (1 allocation: 32 bytes)
(1.0, 2.0, 3.0)

julia> f((2.0,1.0,3.0))
  10.511 ns (0 allocations: 0 bytes)
(1.0, 2.0, 3.0)

@Jutho
Copy link
Owner

Jutho commented Sep 13, 2023

Ok, if it does not change the efficiency of the homogeneous case, I am willing to accept this change. Out of curiosity, where do you need this? I mostly had homogeneous tuples in mind (in particular things like multidimensional indices, sizes, ranges, … ) when developing this package.

@putianyi889
Copy link
Contributor Author

putianyi889 commented Sep 13, 2023

just an example (not exactly my application but similar idea). In some cases we want to sort the arguments before further processing. Converting to a vector hurts performance too much.

julia> TupleTools.sort((zeros(3),zeros(3,3)),by=ndims)
ERROR: MethodError: no method matching _split(::Tuple{Vector{Float64}, Matrix{Float64}})

Closest candidates are:
  _split(::Tuple{Vararg{T, N}} where T) where N
   @ TupleTools C:\Users\pty\.julia\dev\TupleTools\src\TupleTools.jl:253

Stacktrace:
 [1] _sort
   @ C:\Users\pty\.julia\packages\TupleTools\ecrzx\src\TupleTools.jl:245 [inlined]
 [2] sort(t::Tuple{Vector{Float64}, Matrix{Float64}}; lt::Function, by::Function, rev::Bool)
   @ TupleTools C:\Users\pty\.julia\dev\TupleTools\src\TupleTools.jl:243
 [3] top-level scope
   @ REPL[86]:1

julia> isa((zeros(3),zeros(3,3)),NTuple)
false

@putianyi889
Copy link
Contributor Author

if you are still curious 👀

@Jutho
Copy link
Owner

Jutho commented Sep 13, 2023

Ok looks good. I'll merge and tag a new version.

@Jutho Jutho merged commit 8e9d0c4 into Jutho:master Sep 13, 2023
12 checks passed
@Jutho
Copy link
Owner

Jutho commented Sep 13, 2023

Ok, v1.4 will soon be in the General Registry.

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