Skip to content
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

Merged

Conversation

fingolfin
Copy link
Member

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.

Copy link

codecov bot commented Jan 17, 2025

Codecov Report

Attention: Patch coverage is 94.59459% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.28%. Comparing base (3b6fb3f) to head (deff7a7).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
src/flint/fmpq_mpoly.jl 94.59% 2 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@fingolfin 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 fingolfin force-pushed the mh/mul-fmpq_mpoly-by-int-to-fmpz_mpoly branch 2 times, most recently from 327751b to 1f40354 Compare January 21, 2025 08:27
@fingolfin fingolfin marked this pull request as ready for review January 27, 2025 10:43
@fingolfin fingolfin requested a review from lgoettgens January 27, 2025 10:48
@fingolfin
Copy link
Member Author

This should be ready for review now

@fingolfin fingolfin force-pushed the mh/mul-fmpq_mpoly-by-int-to-fmpz_mpoly branch 4 times, most recently from 0c5d9ff to b0f8aa3 Compare February 24, 2025 22:35
nfields::Cint
ord::Int
nfields::Int
ord::Cint
Copy link
Member Author

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)
@fingolfin fingolfin force-pushed the mh/mul-fmpq_mpoly-by-int-to-fmpz_mpoly branch from b0f8aa3 to deff7a7 Compare February 24, 2025 22:48
@fingolfin
Copy link
Member Author

I addressed all issues with this PR I was/am aware of

@lgoettgens lgoettgens merged commit 1945331 into Nemocas:master Feb 25, 2025
25 checks passed
@fingolfin fingolfin deleted the mh/mul-fmpq_mpoly-by-int-to-fmpz_mpoly branch February 25, 2025 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants