JuliaGeometry / Quaternions.jl

A Julia implementation of quaternions
https://juliageometry.github.io/Quaternions.jl
MIT License
116 stars 37 forks source link

Add `Base.hash(::Quaternion, ::UInt)` #136

Closed hyrodium closed 7 months ago

hyrodium commented 10 months ago

This PR fixes #135.

Before this PR

julia> using Quaternions

julia> isequal(1, complex(1))
true

julia> 1 |> hash
0x5bca7c69b794f8ce

julia> complex(1) |> hash
0x5bca7c69b794f8ce

julia> isequal(1, quat(1))  # isequal(a,b) == true should imply `hash(a) == hash(b)`
true

julia> quat(1) |> hash
0x37cb992a8d339608

julia> unique(Complex[complex(2), complex(big"2")])
1-element Vector{Complex}:
 2 + 0im

julia> unique(Quaternion[quat(2), quat(big"2")])
2-element Vector{Quaternion}:
  Quaternion{Int64}(2, 0, 0, 0)
 Quaternion{BigInt}(2, 0, 0, 0)

After this PR

julia> using Quaternions

julia> isequal(1, complex(1))
true

julia> 1 |> hash
0x5bca7c69b794f8ce

julia> complex(1) |> hash
0x5bca7c69b794f8ce

julia> isequal(1, quat(1))  # isequal(a,b) == true should imply `hash(a) == hash(b)`
true

julia> quat(1) |> hash
0x5bca7c69b794f8ce

julia> unique(Complex[complex(2), complex(big"2")])
1-element Vector{Complex}:
 2 + 0im

julia> unique(Quaternion[quat(2), quat(big"2")])
1-element Vector{Quaternion}:
 Quaternion{Int64}(2, 0, 0, 0)
codecov[bot] commented 10 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (89232ef) 100.00% compared to head (eab2f56) 100.00%. Report is 6 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #136 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 2 2 Lines 162 175 +13 ========================================= + Hits 162 175 +13 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

hyrodium commented 10 months ago

Hmm, not sure why the Invalidation test failed..

image

https://github.com/JuliaGeometry/Quaternions.jl/actions/runs/6814371318/job/18531083622?pr=136

hyrodium commented 9 months ago

@ranocha Do you know why the Invalidation check failed?

hyrodium commented 9 months ago

The invalidation failure was because the number of uinvalidated(invalidations) has increased.

Before this PR

julia> using SnoopCompile, SnoopCompileCore

julia> invalidations = @snoopr begin using Quaternions end;

julia> uinvalidated(invalidations)
Set{Core.MethodInstance} with 1 element:
  MethodInstance for promote_rule(::Type, ::Type)

julia> inv_owned = length(filtermod(Quaternions, invalidation_trees(invalidations)))
0

After this PR

julia> using SnoopCompile, SnoopCompileCore

julia> invalidations = @snoopr begin using Quaternions end;

julia> uinvalidated(invalidations)
Set{Core.MethodInstance} with 2 elements:
  MethodInstance for hash(::Any, ::UInt64)
  MethodInstance for promote_rule(::Type, ::Type)

julia> inv_owned = length(filtermod(Quaternions, invalidation_trees(invalidations)))
0

But it seems this is not problematic because inv_owned is zero.

(The above code are from https://github.com/julia-actions/julia-invalidations/blob/main/action.yml.)

ranocha commented 9 months ago

Based on your results above, it looks like there are some new invalidations caused by the new specialization. You don't invalidate your own functions but something that's using hash and is badly inferred. This may impact the latency when loading Quaternions.jl. Of course, this doesn't mean that you cannot go ahead with this PR. It would be a nice contribution to the ecosystem to check where the invalidations are coming from and trying to fix them 🙂

hyrodium commented 9 months ago

I see. Thank you for the detailed explanation!

hyrodium commented 7 months ago

I'll merge this PR in a few days if there are no objections.