-
Notifications
You must be signed in to change notification settings - Fork 126
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
Speed up is_homogeneous
#4119
Speed up is_homogeneous
#4119
Conversation
f692e0e
to
0e78024
Compare
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.
If it works, it works. Thanks!
I now get 10 respectively 12 allocations in the example. |
Some measurements taking the example from the `is_homogeneous` docstring: R, (x, y, z) = graded_polynomial_ring(QQ, ["x", "y", "z"], [1, 2, 3]) f = x^2+y*z W = [1 2 1 0; 3 4 0 1] S, (w, x, y, z) = graded_polynomial_ring(QQ, ["w", "x", "y", "z"], W) F = w^3*y^3*z^3 + w^2*x*y^2*z^2 + w*x^2*y*z + x^3 Before: julia> @Btime is_homogeneous(f); 1.533 μs (62 allocations: 2.28 KiB) julia> @Btime is_homogeneous(F); 3.932 μs (152 allocations: 5.53 KiB) After: julia> @Btime is_homogeneous(f); 334.071 ns (10 allocations: 416 bytes) julia> @Btime is_homogeneous(F); 556.150 ns (12 allocations: 656 bytes)
8d27ade
to
e7e334f
Compare
Maybe we wait for a release of thofma/Hecke.jl#1619 and then can simplify the comments away? |
But that then also needs to go into a new Hecke release, and then more benchmarking should be done to verify it doesn't get slower again etc. I'd rather have this in fact sooner. IMHO the "simplification" is not as important as the speed up. |
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.
Alright. Then let's get this in
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4119 +/- ##
=======================================
Coverage 84.65% 84.66%
=======================================
Files 626 626
Lines 84313 84316 +3
=======================================
+ Hits 71377 71382 +5
+ Misses 12936 12934 -2
|
Hecke 0.34.3 is now released with the required changes. |
I now adapted to the new Hecke functions, and verified that this does not increase the allocations nor the runtime of the example above. @fingolfin if you are happy with this, please merge |
Some measurements taking the example from the `is_homogeneous` docstring: R, (x, y, z) = graded_polynomial_ring(QQ, ["x", "y", "z"], [1, 2, 3]) f = x^2+y*z W = [1 2 1 0; 3 4 0 1] S, (w, x, y, z) = graded_polynomial_ring(QQ, ["w", "x", "y", "z"], W) F = w^3*y^3*z^3 + w^2*x*y^2*z^2 + w*x^2*y*z + x^3 Before: julia> @Btime is_homogeneous(f); 1.533 μs (62 allocations: 2.28 KiB) julia> @Btime is_homogeneous(F); 3.932 μs (152 allocations: 5.53 KiB) After: julia> @Btime is_homogeneous(f); 334.071 ns (10 allocations: 416 bytes) julia> @Btime is_homogeneous(F); 556.150 ns (12 allocations: 656 bytes) --------- Co-authored-by: Lars Göttgens <[email protected]>
Some measurements taking the example from the
is_homogeneous
docstring:Before:
After:
I will make a separate PR for Hecke which provides
addmul!
,zero!
and more forFinGenAbGroupElem
.Also I think there are more places that could be similarly improved, e.g.
homogeneous_components
. However, unlikeis_homogeneous
they did not stick out in some timing profiles on CI tests we made this week.