-
Notifications
You must be signed in to change notification settings - Fork 162
Make perm(::Vector{Int}) constructor and application of permutations to integers faster and allocate less
#5476
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
base: master
Are you sure you want to change the base?
Conversation
ThomasBreuer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Just for curiosity:
For n of type T different from Int, now we compute n^g by first converting n to Int, then computing the image of that Int, and then converting the result to T.
Is this cheaper than converting the unsafe_load result directly to T?
lgoettgens
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests fail
7a03e93 to
a883365
Compare
| return x | ||
| end | ||
|
|
||
| function _make_gap_perm(::Type{T}, deg::Int, L::AbstractVector{<:IntegerUnion}) where {T <: Union{UInt16, UInt32}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering if this function should belong to Oscar or rather to GAP.jl as it contains ccalls into libgap. That seems to be an implementation detail of GAP.jl. But no strong opinion on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the long run, yes, maybe it should go to GAP.jl. But I'd like to first have something working, and I expect a few similar changes and tweaks to be needed first.
734b705 to
0625c46
Compare
Before:
julia> make_test_data(n) = (symmetric_group(n), vcat([n],1:n-1));
julia> testit(pi) = [ i^pi for i in 1:degree(pi) ];
julia> G, L = make_test_data(1000); pi = G(L); @b testit(pi)
8.222 μs (492 allocs: 15.703 KiB)
julia> G, L = make_test_data(10000); pi = G(L); @b testit(pi)
98.916 μs (9492 allocs: 244.328 KiB)
julia> G, L = make_test_data(100000); pi = G(L); @b testit(pi)
1.069 ms (99492 allocs: 2.299 MiB)
julia> G, L = make_test_data(1000); @b $G($L)
12.834 μs (4 allocs: 10.016 KiB)
julia> G, L = make_test_data(10000); @b $G($L)
124.500 μs (4 allocs: 97.906 KiB)
julia> G, L = make_test_data(100000); @b $G($L)
1.235 ms (4 allocs: 1.145 MiB)
After:
julia> make_test_data(n) = (symmetric_group(n), vcat([n],1:n-1));
julia> testit(pi) = [ i^pi for i in 1:degree(pi) ];
julia> G, L = make_test_data(1000); pi = G(L); @b testit(pi)
1.041 μs (3 allocs: 8.062 KiB)
julia> G, L = make_test_data(10000); pi = G(L); @b testit(pi)
9.833 μs (3 allocs: 96.062 KiB)
julia> G, L = make_test_data(100000); pi = G(L); @b testit(pi)
104.625 μs (3 allocs: 800.062 KiB)
julia> G, L = make_test_data(1000); @b $G($L)
1.905 μs (2 allocs: 2.000 KiB)
julia> G, L = make_test_data(10000); @b $G($L)
16.458 μs (2 allocs: 19.641 KiB)
julia> G, L = make_test_data(100000); @b $G($L)
159.458 μs (2 allocs: 390.766 KiB)
0625c46 to
ac6e83e
Compare
| for i in 1:length(L) | ||
| unsafe_store!(addr, UInt64(L[i]) - 1, i) | ||
| end | ||
| for i in 1+length(L):deg | ||
| unsafe_store!(addr, i - 1, i) | ||
| end | ||
| return perm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still fails because I only did the "unsafe" version which assumes the input is valid.
But the input could contain duplicates or "out of bounds" entries. I'll keep this one as unsafe_perm or so, and add one that performs those checks, just like the one in the GAP kernel (let's see how much time difference then remains)
Before:
After: