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

Conversion of integers to floats in GeoUnit conversion #109

Closed
albert-de-montserrat opened this issue Sep 20, 2023 · 5 comments
Closed

Conversion of integers to floats in GeoUnit conversion #109

albert-de-montserrat opened this issue Sep 20, 2023 · 5 comments

Comments

@albert-de-montserrat
Copy link
Member

albert-de-montserrat commented Sep 20, 2023

Is there any specific reason for converting integers to floats for the convert method corresponding to GeoUnits ?

I am asking because for the stress / strain / viscosity calculations the slowest part is by far the powers with float exponents, when in fact many times most of them are actual integers. Example:

using GeoParams; import GeoParams.fastpow
f, d,  A, FT, E, P, V, R, T, FE, TauII = tuple(rand(11)...)

function f1(f, r, d, p,  A, FT, E, P, V, R, T, FE, TauII, n)
    (fastpow(FT * TauII, n) *fastpow(f, r) * fastpow(d, p) * A * FT * exp((-E - P * V) / (R * T)) * inv(FE))
end
julia> @btime f1($(f, 1.0, d, 2.0,  A, FT, E, P, V, R, T, FE, TauII, 3.0)...)
  16.658 ns (0 allocations: 0 bytes)
0.009911001392750043

julia> @btime f1($(f, 1, d, 2,  A, FT, E, P, V, R, T, FE, TauII, 3)...)
  13.388 ns (0 allocations: 0 bytes)
0.009911001392750043

I would like to not force the conversion to floats in GeoUnits if possible, hopefully to scratch some performance gain here since computing the viscosity with diffusion and/or dislocation creep turns out to be one of the most expensive parts of the PT iterations.

@boriskaus
Copy link
Member

I’m fine if you undo this automatic conversion

@boriskaus
Copy link
Member

If this is the slowest part, should we perhaps precompute many of these factors? Grain size will rarely be set as spatially variable so one could precompute that. Same with many of the parameters in the thermal part

@albert-de-montserrat
Copy link
Member Author

Pre-computing fastpow(f, r) and fastpow(d, p) would certainly help a lot, by doing that f1 goes down to about 6 ns, as long as n is an integer.

@boriskaus
Copy link
Member

the issue is to find a way to tell the code when to precompute vs. when to do the full computation.

@albert-de-montserrat
Copy link
Member Author

albert-de-montserrat commented Sep 21, 2023

A somehow less intrusive change, which seems to perform almost as well as precomputing those factors, is adding a couple of checks:

function f2(f, r, d, p,  A, FT, E, P, V, R, T, FE, TauII, n)
    fr = if iszero(r)
        1.0
    elseif isone(r)
        f
    else
        fastpow(f, r)
    end
    dp = if iszero(p)
        1.0
    elseif isone(p)
        d
    else
        fastpow(d, p)
    end
    (fastpow(FT * TauII, n) * fr * dp * A * FT * exp((-E - P * V) / (R * T)) * inv(FE))
end
In [9]: @btime f1($(f, 1.0, d, 0.0,  A, FT, E, P, V, R, T, FE, TauII, 3.0)...)
  19.038 ns (0 allocations: 0 bytes)
0.0007530828398154311

In [10]: @btime f2($(f, 1.0, d, 0.0,  A, FT, E, P, V, R, T, FE, TauII, 3.0)...)        
  14.300 ns (0 allocations: 0 bytes)
0.0007530828398154311

The boost of pre-computing inv(R * T) seems minimal, in particular when n isa AbstractFloat), so I will leave this part as it is now.

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

No branches or pull requests

2 participants