-
Notifications
You must be signed in to change notification settings - Fork 62
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
Optimize map_coefficients, change_base_ring for some inputs #1994
Optimize map_coefficients, change_base_ring for some inputs #1994
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1994 +/- ##
==========================================
+ Coverage 88.24% 88.28% +0.03%
==========================================
Files 100 100
Lines 36740 36767 +27
==========================================
+ Hits 32422 32459 +37
+ Misses 4318 4308 -10 ☔ View full report in Codecov by Sentry. |
b4f481a
to
550e2e6
Compare
327751b
to
1f40354
Compare
This should be ready for review now |
0c5d9ff
to
b0f8aa3
Compare
nfields::Cint | ||
ord::Int | ||
nfields::Int | ||
ord::Cint |
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.
These field sizes were wrong; it didn't matter for us in practice but it showed up when I used dump
to understand why sort_terms!
didn't do what I thought it would...
julia> dump(polynomial_ring(QQ, [:u,:v]; internal_ordering = :degrevlex)[1]; maxdepth=1)
QQMPolyRing
nvars: Int64 2
nfields: Int32 3
ord: Int64 4294967298
deg: Int32 1
rev: Int32 0
lut: NTuple{64, Int64}
lut1: NTuple{64, UInt8}
S: Array{Symbol}((2,))
__attrs: #undef
Namely if the input polynomial is a `QQMPolyRingElem` and the transformation "function" is `ZZ` resp. a `fpField`. Before this patch: julia> Qxy,(x,y) = QQ[:x,:y]; Zxy,_ = ZZ[:u,:v]; julia> f = 11*(x^2/2 + ZZ(3)^50*x*y^5/7 + y^6/12); julia> @b $f*84 113.861 ns (4 allocs: 200 bytes) julia> @b change_base_ring($ZZ, $f*84) 1.181 μs (45.96 allocs: 1.640 KiB) julia> @b change_base_ring($ZZ, $f*84; parent = $Zxy) 666.667 ns (24.25 allocs: 810 bytes) julia> @b map_coefficients($ZZ, $f*84; parent = $Zxy) 631.250 ns (24 allocs: 808 bytes) After this patch: julia> Qxy,(x,y) = QQ[:x,:y]; Zxy,_ = ZZ[:u,:v]; julia> f = 11*(x^2/2 + ZZ(3)^50*x*y^5/7 + y^6/12); julia> @b $f*84 113.693 ns (4 allocs: 200 bytes) julia> @b change_base_ring($ZZ, $f*84) 694.057 ns (29 allocs: 1.211 KiB) julia> @b change_base_ring($ZZ, $f*84; parent = $Zxy) 220.339 ns (8 allocs: 376 bytes) julia> @b map_coefficients($ZZ, $f*84; parent = $Zxy) 217.712 ns (8 allocs: 376 bytes) As a bonus we now also have a special `mul!` method which is even faster but is not suitable for the faint of heart: julia> @b mul!($Zxy(), $f, 84) 92.590 ns (4 allocs: 176 bytes)
b0f8aa3
to
deff7a7
Compare
I addressed all issues with this PR I was/am aware of |
Namely if the input polynomial is a
QQMPolyRingElem
and the transformation "function" isZZ
resp. afpField
.Before this patch:
After this patch:
As a bonus we now also have a special
mul!
method which is even faster but is not suitable for the faint of heart:This needs work, at least tests. But I didn't want to forget this, plus this way I can get feedback.