Optimize map_coefficients, change_base_ring for some inputs #1994

fingolfin · 2025-01-17T17:07:38Z

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
160.000 ns (4 allocs: 200 bytes)

julia> @b change_base_ring($ZZ, $f*84)
1.559 μs (45 allocs: 1.633 KiB)

julia> @b change_base_ring($ZZ, $f*84; parent = $Zxy)
1.002 μs (24 allocs: 808 bytes)

julia> @b map_coefficients($ZZ, $f*84; parent = $Zxy)
996.680 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
111.693 ns (4 allocs: 200 bytes)

julia> @b change_base_ring($ZZ, $f*84)
733.806 ns (30 allocs: 1.227 KiB)

julia> @b change_base_ring($ZZ, $f*84; parent = $Zxy)
242.117 ns (9 allocs: 392 bytes)

julia> @b map_coefficients($ZZ, $f*84; parent = $Zxy)
249.607 ns (9 allocs: 392 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)
129.041 ns (5 allocs: 192 bytes)

This needs work, at least tests. But I didn't want to forget this, plus this way I can get feedback.

codecov · 2025-01-17T17:18:30Z

Codecov Report

Attention: Patch coverage is 51.51515% with 16 lines in your changes missing coverage. Please review.

Project coverage is 88.30%. Comparing base (fb7f84d) to head (47be76c).

Files with missing lines	Patch %	Lines
src/flint/fmpq_mpoly.jl	51.51%	16 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1994      +/-   ##
==========================================
- Coverage   88.34%   88.30%   -0.04%     
==========================================
  Files          98       98              
  Lines       36211    36232      +21     
==========================================
+ Hits        31991    31996       +5     
- Misses       4220     4236      +16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/flint/fmpq_mpoly.jl

fingolfin · 2025-01-21T08:14:55Z

src/HeckeMoreStuff.jl

@@ -182,21 +182,6 @@ function Base.hash(f::zzModMPolyRingElem, h::UInt)
  return UInt(1) # TODO: enhance or throw error
 end

-function AbstractAlgebra.map_coefficients(F::fpField, f::QQMPolyRingElem; parent=polynomial_ring(F, nvars(parent(f)), cached=false)[1])


the parent computation here was wrong as it ignored the internal ordering

@b

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 160.000 ns (4 allocs: 200 bytes) julia> @b change_base_ring($ZZ, $f*84) 1.559 μs (45 allocs: 1.633 KiB) julia> @b change_base_ring($ZZ, $f*84; parent = $Zxy) 1.002 μs (24 allocs: 808 bytes) julia> @b map_coefficients($ZZ, $f*84; parent = $Zxy) 996.680 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 111.693 ns (4 allocs: 200 bytes) julia> @b change_base_ring($ZZ, $f*84) 733.806 ns (30 allocs: 1.227 KiB) julia> @b change_base_ring($ZZ, $f*84; parent = $Zxy) 242.117 ns (9 allocs: 392 bytes) julia> @b map_coefficients($ZZ, $f*84; parent = $Zxy) 249.607 ns (9 allocs: 392 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) 129.041 ns (5 allocs: 192 bytes)

fingolfin · 2025-01-22T09:36:31Z

src/flint/fmpq_mpoly.jl

+                          parent::ZZMPolyRing = AbstractAlgebra._change_mpoly_ring(R, parent(f), cached))
+  @req isinteger(_content_ptr(f)) "input polynomial must have integral coefficients"
+  @req ngens(parent) == ngens(Nemo.parent(f)) "parents must have matching numbers of generators"
+  @req internal_ordering(parent) == internal_ordering(Nemo.parent(f)) "parents must have matching internal ordering"


Is this too strict? Maybe instead of erroring out when the orderings don't match we should fall back to the generic code ?

How do other methods of map_coefficients handle this? The one below works for different internal orderings, so IMO this one should do so as well, e.g. by delegating to some more generic approach

Ahh apparently I can use sort_terms! to solve this

(A test case should also be added to verify this works as desired)

fingolfin · 2025-01-27T10:48:54Z

This should be ready for review now

src/flint/fmpq_mpoly.jl

lgoettgens · 2025-01-28T09:47:15Z

src/flint/fmpq_mpoly.jl

+                          parent::ZZMPolyRing = AbstractAlgebra._change_mpoly_ring(R, parent(f), cached))
+  @req isinteger(_content_ptr(f)) "input polynomial must have integral coefficients"
+  @req ngens(parent) == ngens(Nemo.parent(f)) "parents must have matching numbers of generators"
+  @req internal_ordering(parent) == internal_ordering(Nemo.parent(f)) "parents must have matching internal ordering"


How do other methods of map_coefficients handle this? The one below works for different internal orderings, so IMO this one should do so as well, e.g. by delegating to some more generic approach

fingolfin · 2025-01-29T23:17:35Z

src/flint/fmpq_mpoly.jl

+  return mul!(zero(parent), f, 1)
+end
+
+function map_coefficients(F::fpField, f::QQMPolyRingElem;


Apparently this function is untested so a test should be added (TODO)

fingolfin · 2025-01-29T23:20:43Z

src/flint/fmpq_mpoly.jl

+  m = inv(d)
+  ctx = MPolyBuildCtx(parent)
+  for x in zip(coefficients(f), exponent_vectors(f))
+    el = numerator(x[1] * dF)


Could optimize this further by using _zpoly_ptr (to avoid converting the internal fmpz coefficients into fmpq, and then back via a multiplication)

thofma · 2025-01-30T06:13:25Z

src/flint/fmpq_mpoly.jl

+# obviously this only works if the integers is a multiple of the denominator of the polynomial
+function mul!(a::ZZMPolyRingElem, b::QQMPolyRingElem, c::IntegerUnion)
+  x = QQFieldElem()
+  GC.@preserve a b x begin


Suggested change

GC.@preserve a b x begin

GC.@preserve b x begin

thofma reviewed Jan 17, 2025

View reviewed changes

src/flint/fmpq_mpoly.jl Outdated Show resolved Hide resolved

src/flint/fmpq_mpoly.jl Outdated Show resolved Hide resolved

fingolfin force-pushed the mh/mul-fmpq_mpoly-by-int-to-fmpz_mpoly branch 2 times, most recently from b4f481a to 550e2e6 Compare January 21, 2025 08:06

fingolfin commented Jan 21, 2025

View reviewed changes

fingolfin force-pushed the mh/mul-fmpq_mpoly-by-int-to-fmpz_mpoly branch from 550e2e6 to 327751b Compare January 21, 2025 08:15

fingolfin force-pushed the mh/mul-fmpq_mpoly-by-int-to-fmpz_mpoly branch from 327751b to 1f40354 Compare January 21, 2025 08:27

input validation

5a88ed0

fingolfin commented Jan 22, 2025

View reviewed changes

fingolfin marked this pull request as ready for review January 27, 2025 10:43

remove @Assert

47be76c

fingolfin requested a review from lgoettgens January 27, 2025 10:48

fingolfin mentioned this pull request Jan 27, 2025

Make use of new Nemo method mul!(a::ZZMPolyRingElem, b::QQMPolyRingElem, c::IntegerUnion) once available oscar-system/GenericCharacterTables.jl#249

Open

lgoettgens reviewed Jan 28, 2025

View reviewed changes

fingolfin commented Jan 29, 2025

View reviewed changes

thofma reviewed Jan 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize map_coefficients, change_base_ring for some inputs #1994

Optimize map_coefficients, change_base_ring for some inputs #1994

fingolfin commented Jan 17, 2025

codecov bot commented Jan 17, 2025 •

edited

Loading

fingolfin Jan 21, 2025

fingolfin Jan 22, 2025

lgoettgens Jan 28, 2025

fingolfin Jan 29, 2025

fingolfin Jan 29, 2025

fingolfin commented Jan 27, 2025

lgoettgens Jan 28, 2025

fingolfin Jan 29, 2025

fingolfin Jan 29, 2025

thofma Jan 30, 2025

Optimize map_coefficients, change_base_ring for some inputs #1994

Are you sure you want to change the base?

Optimize map_coefficients, change_base_ring for some inputs #1994

Conversation

fingolfin commented Jan 17, 2025

codecov bot commented Jan 17, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin commented Jan 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 17, 2025 •

edited

Loading