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

Simplify some functions #19

Merged
merged 3 commits into from
Mar 20, 2017
Merged

Simplify some functions #19

merged 3 commits into from
Mar 20, 2017

Conversation

KristofferC
Copy link
Collaborator

@KristofferC KristofferC commented Mar 12, 2017

TODO:

  • fix more functions
  • benchmark

@inbounds return $TensorType($expr)
end
@inline function otimes{dim}(S1::SecondOrderTensor{dim}, S2::SecondOrderTensor{dim})
TensorType = getreturntype(otimes, get_base(typeof(S1)), get_base(typeof(S2)))
Copy link
Member

@fredrikekre fredrikekre Mar 12, 2017

Choose a reason for hiding this comment

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

This will only be fast if getreturntype is @pure
Edit: Should have kept scrolling.

@KristofferC
Copy link
Collaborator Author

We could perhaps have some "reduce" function to simplify other things

Tt = get_base(typeof(S))
tr = trace(S) / 3
Tt(
@inline function(i, j)
Copy link
Member

Choose a reason for hiding this comment

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

This function will return different eltype for Int tensors.

Copy link
Member

Choose a reason for hiding this comment

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

julia> Tensor{2, 3}((i, j)-> i == j ? 1 : 1.0)
ERROR: MethodError: Cannot `convert` an object of type Tuple{Int64,Float64,Float64,Float64,Int64,Float64,Float64,Float64,Int64} to an object of type Tensors.Tensor{2,3,T<:Real,M}
This may have arisen from a call to the constructor Tensors.Tensor{2,3,T<:Real,M}(...),
since type constructors fall back to convert methods.
 in Tensors.Tensor{2,3,T<:Real,M}(::Tuple{Int64,Float64,Float64,Float64,Int64,Float64,Float64,Float64,Int64}) at ./sysimg.jl:53
 in macro expansion at /home/fredrik/Dropbox/Programming/Tensors.jl/src/constructors.jl:15 [inlined]
 in Tensors.Tensor{2,3,T<:Real,M}(::##1#2) at /home/fredrik/Dropbox/Programming/Tensors.jl/src/constructors.jl:5

Copy link
Collaborator Author

@KristofferC KristofferC Mar 12, 2017

Choose a reason for hiding this comment

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

Yep. I tried:

@inline function dev(S::SecondOrderTensor)
    Tt = get_base(typeof(S))
    tr = trace(S) / 3
    T = typeof(one(eltype(S)) / 3)
    Tt(
        @inline function(i, j)
            @inbounds if i == j
                return T(S[i,j] - tr)
            else
                return T(S[i,j])
            end
        end
    )
end

but it is slow for some reason.

Copy link
Member

Choose a reason for hiding this comment

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

Could do this:

@inline function dev{dim, T}(S::SecondOrderTensor{dim, T})
    Tt = get_base(typeof(S))
    tr = trace(S) / 3
    R = promote_type(T, typeof(tr))
    Tt(
        @inline function(i, j)
            @inbounds if i == j
                return R(S[i,j] - tr)
            else
                return R(S[i,j])
            end
        end
    )
end

Copy link
Member

Choose a reason for hiding this comment

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

Edit, yea its really slow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like Tt is not inferred..

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't work on master either so

julia> A = rand(Tensor{2, 3, Int});

julia> dev(A)
ERROR: MethodError: Cannot `convert` an object of type Tuple{Float64,Int64,Int64,Int64,Float64,Int64,Int64,Int64,Float64} to an object of type Tensors.Tensor{2,3,T<:Real,M}
This may have arisen from a call to the constructor Tensors.Tensor{2,3,T<:Real,M}(...),
since type constructors fall back to convert methods.
 in macro expansion at /home/fredrik/Dropbox/Programming/Tensors.jl/src/math_ops.jl:275 [inlined]
 in dev(::Tensors.Tensor{2,3,Int64,9}) at /home/fredrik/Dropbox/Programming/Tensors.jl/src/math_ops.jl:267

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I put it back since it fails on master

Copy link
Member

Choose a reason for hiding this comment

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

We could perhaps limit eltype for this method to AbstractFloat?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ideally it should just promote but I dunno why it gets slow then...

src/transpose.jl Outdated
$(Expr(:meta, :inline))
@inbounds return Tensor{4, dim}($expr)
end
@inline function majortranspose{dim}(S::Tensor{4, dim})
Copy link
Member

Choose a reason for hiding this comment

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

This method should have S::FourthOrderTensor{dim} as input but always return Tensor{4, dim} so just change the input type signature.

@KristofferC
Copy link
Collaborator Author

Did some more functions.. Using the function stuff is really powerful.

@KristofferC
Copy link
Collaborator Author

Jesus we have a lot of benchmarks...

src/transpose.jl Outdated
@inbounds return Tensor{2, dim}($expr)
end
@inline function Base.transpose{dim}(S::Tensor{2, dim})
Tensor{2, dim}(@inline function(i, j) @inbounds v = S[j,i]; v; end)
Copy link
Member

Choose a reason for hiding this comment

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

@inboundsret ?

@fredrikekre
Copy link
Member

This is great!

Jesus we have a lot of benchmarks...

Yea, better safe than sorry :p

@KristofferC
Copy link
Collaborator Author

Benchmarks here: https://gist.github.com/KristofferC/8f423c6d4b2c1f614b3ff7f03fa7c394

I did not find any difference in the generated code for anything that looked like it could be real regressions.

@fredrikekre
Copy link
Member

I tested some cases too, didn't see any difference. Benchmarks really are weird...

@codecov-io
Copy link

codecov-io commented Mar 13, 2017

Codecov Report

Merging #19 into master will decrease coverage by 0.18%.
The diff coverage is 96.96%.

@@            Coverage Diff             @@
##           master      #19      +/-   ##
==========================================
- Coverage   97.62%   97.44%   -0.19%     
==========================================
  Files          14       14              
  Lines         970      899      -71     
==========================================
- Hits          947      876      -71     
  Misses         23       23
Impacted Files Coverage Δ
src/constructors.jl 97.53% <100%> (+1.14%) ⬆️
src/tensor_ops_errors.jl 100% <100%> (ø) ⬆️
src/math_ops.jl 100% <100%> (ø) ⬆️
src/tensor_products.jl 100% <100%> (ø) ⬆️
src/transpose.jl 100% <100%> (ø) ⬆️
src/symmetric.jl 100% <100%> (ø) ⬆️
src/basic_operations.jl 92.85% <66.66%> (-7.15%) ⬇️
src/utilities.jl 98.27% <95%> (+0.09%) ⬆️

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 7728952...ed841b0. Read the comment docs.

@KristofferC
Copy link
Collaborator Author

Pfft 36 min on Travis 0.6. Is this due to a change in Base or this PR?

@fredrikekre
Copy link
Member

Locally on v06:

278.792679 seconds (1.01 G allocations: 21.509 GiB, 2.43% gc time) # master
479.304849 seconds (3.57 G allocations: 63.570 GiB, 3.17% gc time) # PR

@KristofferC
Copy link
Collaborator Author

Yuck... hmm.

@fredrikekre
Copy link
Member

fredrikekre commented Mar 13, 2017

JuliaLang/julia#18077 ? But upgraded to 0.6 from 0.5 :)

@KristofferC
Copy link
Collaborator Author

KristofferC commented Mar 18, 2017

 Section               ncalls     time   %tot     avg     alloc   %tot      avg
 ──────────────────────────────────────────────────────────────────────────────
 tensor ops                 1     923s  57.6%    923s    131GiB  61.9%   131GiB
   symmetric/skew-s...      9     665s  41.5%   73.9s    101GiB  47.5%  11.2GiB
   transpose                9     159s  9.93%   17.7s   22.4GiB  10.5%  2.49GiB
   outer product            9    73.6s  4.59%   8.18s   7.26GiB  3.42%   826MiB
   double contraction       9    21.9s  1.37%   2.43s    779MiB  0.36%  86.6MiB
   dot products             9    1.82s  0.11%   202ms   55.1MiB  0.03%  6.12MiB
   special                  9    1.12s  0.07%   124ms   36.7MiB  0.02%  4.08MiB
   cross product            9    186ms  0.01%  20.7ms   3.96MiB  0.00%   450KiB
   rotation                 9    362μs  0.00%  40.2μs   40.6KiB  0.00%  4.52KiB
 constrct Arr               1     281s  17.5%    281s   41.8GiB  19.7%  41.8GiB
 AD                         1     149s  9.30%    149s   11.9GiB  5.61%  11.9GiB
   dim 3                    1     129s  8.06%    129s   11.1GiB  5.25%  11.1GiB
     hessian                2    70.2s  4.38%   35.1s   6.79GiB  3.20%  3.39GiB
     2nd tensor grad        2    23.3s  1.45%   11.6s   1.60GiB  0.75%   817MiB
     scalar grad            2    2.05s  0.13%   1.03s    113MiB  0.05%  56.4MiB
   dim 2                    1    14.3s  0.89%   14.3s    625MiB  0.29%   625MiB
     hessian                2    6.39s  0.40%   3.19s    293MiB  0.13%   147MiB
     2nd tensor grad        2    3.39s  0.21%   1.69s    132MiB  0.06%  65.8MiB
     scalar grad            2    1.15s  0.07%   573ms   41.2MiB  0.02%  20.6MiB
   dim 1                    1    5.53s  0.34%   5.53s    163MiB  0.07%   163MiB
     hessian                2    2.65s  0.17%   1.33s   81.9MiB  0.04%  40.9MiB
     2nd tensor grad        2    1.09s  0.07%   543ms   24.8MiB  0.01%  12.4MiB
     scalar grad            2    549ms  0.03%   274ms   16.0MiB  0.01%  8.00MiB
 indexing                   1     136s  8.47%    136s   20.9GiB  9.84%  20.9GiB
 tensor identities          1    44.9s  2.80%   44.9s   3.85GiB  1.81%  3.85GiB
 constructors               1    22.8s  1.42%   22.8s    700MiB  0.32%   700MiB
 simple math                1    17.1s  1.07%   17.1s    724MiB  0.33%   724MiB
 norm, trace, det, ...      1    13.0s  0.81%   13.0s    484MiB  0.22%   484MiB
 promotion/conversion       1    7.32s  0.46%   7.32s    301MiB  0.14%   301MiB
 base vectors               1    3.66s  0.23%   3.66s    137MiB  0.06%   137MiB
 diagm, one                 1    3.41s  0.21%   3.41s   93.9MiB  0.04%  93.9MiB
 constrct func              1    2.64s  0.16%   2.64s   96.5MiB  0.04%  96.5MiB
 exceptions                 1   2.14ms  0.00%  2.14ms   7.89KiB  0.00%  7.89KiB
 ──────────────────────────────────────────────────────────────────────────────

symmetric /skew symmetric 600 seconds!, one master, 3.52 seconds.

construct from array: 13.8s -> 281s

@fredrikekre
Copy link
Member

Seems to be extremely hard on inference with this method...

@KristofferC
Copy link
Collaborator Author

I don't get it though.. on 0.5.1:

<< using Tensors

<< a  = rand(Tensor{4,3});

<< @time symmetric(a);
 51.641367 seconds (68.55 M allocations: 1.077 GB, 0.48% gc time)

so why is it fast when doing the tests?

@fredrikekre
Copy link
Member

That time is just ridiculous LOL

@fredrikekre
Copy link
Member

I tried typeasserting the functions but didn’t make a difference.

@KristofferC
Copy link
Collaborator Author

This is fast though:

immutable Tensor{order, dim, T <: Real, M}
    data::NTuple{M, T}
end

@generated function gg{order, dim}((S::Type{Tensor{order, dim}}), f::Function)
    exp = Expr(:tuple, [:(f($i,$j,$k,$l)) for i=1:dim, j=1:dim, k = 1:dim, l = 1:dim]...)
    return quote
        return $exp
    end
end

gg(Tensor{4,3}, (i,j,k,l) -> i+j+k+l)

@KristofferC
Copy link
Collaborator Author

With JuliaLang/julia#21085 tests on this branch are slightly faster than on master.

@KristofferC
Copy link
Collaborator Author

It seems that we can generally avoid using the compute_index stuff to avoid the indexing because the compiler does it by itself.
For example:

@generated function dcontract_muladd{dim}(S1::Tensor{2, dim}, S2::Tensor{2, dim})
    ex1 = Expr[:(S1[$i, $j]) for i in 1:dim, j in 1:dim][:]
    ex2 = Expr[:(S2[$i, $j]) for i in 1:dim, j in 1:dim][:]
    exp = reducer(ex1, ex2, true)
    return quote
        $(Expr(:meta, :inline))
        @inbounds return $exp
    end
end

produces just as good code as the one with compute_index.

@fredrikekre
Copy link
Member

Yea, but for SymmetricTensor we can not do that

@KristofferC
Copy link
Collaborator Author

Why not?

@fredrikekre
Copy link
Member

the for i in 1:dim, j in 1:dim-loop I mean.

@KristofferC
Copy link
Collaborator Author

Ah, no. This also seems to be more expensive for compilation...

@KristofferC KristofferC reopened this Mar 20, 2017
@inbounds return $TensorType($expr)
end
end
Base.fill{T <: AbstractTensor}(el::Number, S::Type{T}) = apply_all(get_base(T), i -> el)
Copy link
Member

Choose a reason for hiding this comment

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

This became really neat.

end
@inline Base.det(t::SecondOrderTensor{1}) = @inboundsret t[1,1]
@inline Base.det(t::SecondOrderTensor{2}) = @inboundsret (t[1,1] * t[2,2] - t[1,2] * t[2,1])
@inline function Base.det(t::SecondOrderTensor{3})
Copy link
Member

Choose a reason for hiding this comment

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

Will this give the same code even for SymmetricTensor?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Appear so, yes.

end
@inline function otimes{dim}(S1::SecondOrderTensor{dim}, S2::SecondOrderTensor{dim})
TensorType = getreturntype(otimes, get_base(typeof(S1)), get_base(typeof(S2)))
TensorType(@inline function(i,j,k,l) @inboundsret S1[i,j] * S2[k,l]; end)
Copy link
Member

Choose a reason for hiding this comment

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

Same here; results in the same code for SymmetricTensor?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm pretty sure checked that but can check again.

Copy link
Member

Choose a reason for hiding this comment

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

As long as compute_index via getindex is elided for Tensor, no reason it shouldn't for SymmetricTensor as well. Just making sure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, exact same code

@@ -7,7 +7,7 @@ end

@testsection "tensor ops" begin
for T in (Float32, Float64, F64), dim in (1,2,3)

println("T = $T, dim = $dim")
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this should be removed? It was included in #23

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is there because sometimes there were time outs for this section because nothing got printed for 10 minutes. I think maybe when Travis is extra slow. So good to have something that prints maybe?

Copy link
Member

Choose a reason for hiding this comment

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

But #23 results in prints just as often?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is no printing there from what I can see?

Copy link
Member

Choose a reason for hiding this comment

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

@KristofferC
Copy link
Collaborator Author

Good to go?

Copy link
Member

@fredrikekre fredrikekre left a comment

Choose a reason for hiding this comment

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

Nice code deletion!

@fredrikekre fredrikekre changed the title WIP: simplify some functions Simplify some functions Mar 20, 2017
@KristofferC KristofferC merged commit d01ee37 into master Mar 20, 2017
@KristofferC KristofferC deleted the kc/simplify branch March 20, 2017 08:13
@fredrikekre
Copy link
Member

I tried this #19 (comment) for all functions. Test time (eg compilation time) went from 120s to 750. It really helps the compiler to compute the linear index by ourselves. Sadly, cause it led to some nice code deletion

@KristofferC
Copy link
Collaborator Author

Perhaps we can make an issue about it...

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.

3 participants