Skip to content

Conversation

@sethaxen
Copy link
Collaborator

Fixes #55. Also fixes #39 as a bonus.

@codecov
Copy link

codecov bot commented Feb 19, 2022

Codecov Report

Merging #56 (0babbf0) into master (4c86b46) will decrease coverage by 0.11%.
The diff coverage is 100.00%.

❗ Current head 0babbf0 differs from pull request most recent head 596749c. Consider uploading reports for the commit 596749c to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #56      +/-   ##
==========================================
- Coverage   55.43%   55.31%   -0.12%     
==========================================
  Files           3        3              
  Lines         359      367       +8     
==========================================
+ Hits          199      203       +4     
- Misses        160      164       +4     
Impacted Files Coverage Δ
src/Quaternion.jl 85.86% <100.00%> (-1.64%) ⬇️

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 4c86b46...596749c. Read the comment docs.

@sethaxen
Copy link
Collaborator Author

sethaxen commented Feb 20, 2022

For reference, this is the heuristic approach I used to identify potentially complex analytic functions in Base:

using FiniteDifferences

fdm = central_fdm(5, 1)

function get_complex_analytic_funs(mod::Module, z=rand(ComplexF64); ignore_defaults=true)
    fnames = names(mod)
    return filter(fnames) do fname
        f = getproperty(mod, fname)
        f isa Union{DataType,UnionAll} && return false
        hasmethod(f, Tuple{ComplexF64}) || return false
        ignore_defaults && hasmethod(f, Tuple{Number}) && return false
        try
            f(z) isa Complex || return false
        catch e
            println("$f errored on input $z")
            return false
        end
        return is_analytic_at(f, z)
    end
end

function is_analytic_at(f, z)
    J = only(jacobian(fdm, f, z))
    # Cauchy–Riemann equations
    return J[1,1]  J[2,2] && J[1, 2]  -J[2, 1]
end
julia> get_complex_analytic_funs(Base) |> sort
27-element Vector{Symbol}:
 :acos
 :acosh
 :asin
 :asinh
 :atan
 :atanh
 :cis
 :cispi
 :cos
 :cosh
 :cospi
 :exp
 :exp10
 :exp2
 :expm1
 :log
 :log10
 :log1p
 :log2
 :round
 :sin
 :sinh
 :sinpi
 :sqrt
 :tan
 :tanh
 :

round should be handled outside of this PR. The rest are extended here.

Of these, cis and cispi are the two that might be confusing for users and could be removed. Because this package chooses a particular embedding of the complex numbers in the quaternions, exp(im*q) is computeable, but this is not the same thing as the extension of cis(z) to the quaternions. In fact, the extension has an intimate relationship with the axis and angle of a rotation in the same way that cis(z) is related to a rotation.

@sethaxen
Copy link
Collaborator Author

sethaxen commented Feb 21, 2022

@hyrodium this is ready for another review.

@hyrodium
Copy link
Collaborator

hyrodium commented Feb 22, 2022

The exp(::Quaternion) is now much faster than current master branch 🚀

Current master

julia> @benchmark exp(Quaternion(0,1,2,3))
BenchmarkTools.Trial: 10000 samples with 991 evaluations.
 Range (min  max):  41.754 ns  75.420 ns  ┊ GC (min  max): 0.00%  0.00%
 Time  (median):     43.341 ns              ┊ GC (median):    0.00%
 Time  (mean ± σ):   43.569 ns ±  2.194 ns  ┊ GC (mean ± σ):  0.00% ± 0.00%

    ▆▂▁▁█▅                                                    ▁
  ▆▇██████▅▆▆▅▇▆▇▇▆▇▇▇▇▇▆▆▆▆▆▅▆▅▅▃▄▄▄▅▄▃▄▃▃▄▃▃▃▄▄▃▃▂▄▃▃▃▃▃▄▄▄ █
  41.8 ns      Histogram: log(frequency) by time      55.6 ns <

 Memory estimate: 0 bytes, allocs estimate: 0.

With this PR

julia> @benchmark exp(Quaternion(0,1,2,3))
BenchmarkTools.Trial: 10000 samples with 999 evaluations.
 Range (min  max):  11.564 ns  350.632 ns  ┊ GC (min  max): 0.00%  0.00%
 Time  (median):     11.894 ns               ┊ GC (median):    0.00%
 Time  (mean ± σ):   12.043 ns ±   5.150 ns  ┊ GC (mean ± σ):  0.00% ± 0.00%

                    █   ▇▇   █    ▂                             
  ▂▂▂▂▂▂▁▁▂▂▂▃▄▅▅▃▅██▅▆▄██▅▄▃█▄▃▂▃█▃▂▂▃▃▁▂▂▂▁▂▁▁▁▁▁▁▁▁▁▂▁▁▂▂▂▂ ▃
  11.6 ns         Histogram: frequency by time         12.4 ns <

 Memory estimate: 0 bytes, allocs estimate: 0.

@sethaxen
Copy link
Collaborator Author

The exp(::Quaternion) is now much faster than current master branch 🚀

It is faster, but not quite that much. You need to declare the quaternion before the benchmark or wrap it in a dollar sign, otherwise the compiler has too much information and cheats.

julia> q = Quaternion(0,1,2,3)
Quaternion{Int64}(0, 1, 2, 3, false)

julia> @btime exp($q)  # master
  53.776 ns (0 allocations: 0 bytes)
QuaternionF64(-0.8252990620752587, -0.15092132721996449, -0.30184265443992897, -0.45276398165989346, true)

julia> @btime exp($q)  # this PR
  39.218 ns (0 allocations: 0 bytes)
QuaternionF64(-0.8252990620752587, -0.15092132721996449, -0.30184265443992897, -0.45276398165989346, true)

Copy link
Collaborator

@hyrodium hyrodium left a comment

Choose a reason for hiding this comment

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

I commented on a few minor points. It seems okay to merge, so I have approved.

@test exp(Quaternion(0, 2, 0, 0)) === Quaternion(cos(2), sin(2), 0, 0, true)
@test exp(Quaternion(0, 0, 2, 0)) === Quaternion(cos(2), 0, sin(2), 0, true)
@test exp(Quaternion(0, 0, 0, 2)) === Quaternion(cos(2), 0, 0, sin(2), true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

These spaces seem accidentally added. It would be better if we have a code style checker on CI.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Which spaces? The newlines? These are present on master.

I agree that it would have been better to have a formatter when the package was started (though none existed then), but I'd rather not add one now. Reformatting the entire package makes git blames to track the history for a bit of code useless.

Copy link
Collaborator

Choose a reason for hiding this comment

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

8 white spaces are added in line 235.

image

We don't have the spaces in the current master branch.

@test exp(Quaternion(0., 0., 0., 2.)) === Quaternion(cos(2), 0, 0, sin(2), true)
@test norm(exp(Quaternion(0., 0., 0., 0.))) 1

There also are 8 white spaces in lines 241, 247, 253, and 258.

image

Copy link
Collaborator

@hyrodium hyrodium Feb 23, 2022

Choose a reason for hiding this comment

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

I'd rather not add one now.

I agree with that. It should not be in this PR.

Can I add an action for formatting with JuliaFormatter.jl? If this sounds good, I'll make a PR.
cf. https://github.com/julia-actions/julia-format/blob/master/workflows/format_check.yml

Note that the current master branch has 2 and 4 white spaces for indent, and that makes confusing for new contributors.
Formatting code may break git diff, but I think it should be done as soon as possible.

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'd say open a PR and let's see how it looks. My concern is that, especially with the indentation, the formatter will touch almost every line of code, which would not be ideal. I'm all for clean, consistent formatting, but being able to explore the history easily is also important.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Extend all analytic functions in base for quaternions in exp() function, eps yields an error when used on an integer

2 participants