Skip to content

Bivariate resultants over nmod via geometric multipoint evaluation - #2822

Open
maelhos wants to merge 28 commits into
flintlib:mainfrom
maelhos:resultant-multipoint-nmod
Open

maelhos wants to merge 28 commits into
flintlib:mainfrom
maelhos:resultant-multipoint-nmod

Conversation

@maelhos

@maelhos maelhos commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Following Issue #2605, this PR adds _gr_poly_resultant_multipoint / gr_poly_resultant_multipoint, a specialization for the resultant for bivariate polynomials, i.e. for the case where the coefficient ring is itself a polynomial ring. The coefficients of both inputs are evaluated at a geometric progression of points, the resultants of the resulting univariate polynomials are computed, and the result is interpolated back, using the geometric evaluation/interpolation added in #2449.

Currently only a base ring of nmod (word-size prime modulus) is supported; fmpz, fmpq and fmpz_mod can follow later (see discussion in the related issue). _gr_poly_resultant dispatches to it whenever it applies, replacing the Sylvester determinant that was previously used for these rings.

Follows the algorithm in PML's lzz_pXY, with the blockwise evaluation of @AntoineBak fork so that memory doesnt grow as n^3.

Two further refinements over PML: the evaluation points are scaled by a random constant (the FLINT geometric progression always starts at 1, so retrying alone would not avoid a leading coefficient vanishing at 1), and only the leading coefficient of the first argument is required not to vanish at the evaluation points.

The timing is unsurprisingly much better than before (on my small Zen 3 laptop)

deg_y deg_x sylvester subres new syl/new sub/new
4 4 0.00013 2.3e-05 1.59e-05 8.2 1.4
4 16 0.000744 0.000104 7.89e-05 9.4 1.3
4 64 0.00313 0.000475 0.000549 5.7 0.9
4 256 0.0107 0.00226 0.00221 4.8 1.0
8 4 0.00403 0.00016 5.33e-05 75.6 3.0
8 16 0.0203 0.000704 0.000495 41.0 1.4
8 64 0.078 0.00328 0.00191 40.8 1.7
8 256 0.285 0.0141 0.00791 36.0 1.8
16 4 0.106 0.00121 0.000431 245.9 2.8
16 16 0.599 0.00514 0.00188 318.6 2.7
16 64 2.1 0.0242 0.00738 284.6 3.3
16 256 - 0.099 0.0309 - 3.2
32 4 3.01 0.00959 0.00204 1477.5 4.7
32 16 17.3 0.0425 0.00799 2162.8 5.3
32 64 - 0.184 0.0313 - 5.9
32 256 - 0.782 0.127 - 6.2
64 4 89.6 0.0748 0.00871 10292.1 8.6
64 16 - 0.338 0.0344 - 9.8
64 64 - 1.44 0.14 - 10.3
64 256 - 6.25 0.58 - 10.8
128 4 - 0.638 0.0419 - 15.2
128 16 - 2.77 0.168 - 16.5
128 64 - 11.9 0.701 - 16.9
128 256 - - 2.89 - -
256 4 - 5.34 0.229 - 23.3
256 16 - 22.8 0.952 - 24.0
256 64 - - 3.84 - -
256 256 - - 15.5 - -

This PR is partially made using Claude Opus 5.

Adds _gr_poly_resultant_multipoint / gr_poly_resultant_multipoint, a
specialization of the resultant for bivariate polynomials, i.e. for the
case where the coefficient ring is itself a polynomial ring. The
coefficients of both inputs are evaluated at a geometric progression of
points, the resultants of the resulting univariate polynomials are
computed, and the result is interpolated back, using the geometric
evaluation/interpolation added in flintlib#2449.

Currently only a base ring of nmod (word-size prime modulus) is
supported; fmpz, fmpq and fmpz_mod can follow later. _gr_poly_resultant
dispatches to it whenever it applies, replacing the Sylvester
determinant that was previously used for these rings.

Follows the algorithm in PML's lzz_pXY, with the blockwise evaluation of
Antoine Bak's fork so that the working space stays proportional to the
input and output sizes: the points are processed in blocks sized to a
fixed memory target, each block being reduced to the same progression
1, q, q^2, ... by a substitution x -> s x on the inputs, so that a
single evaluation precomputation serves for all of them.

Two further refinements over PML: the evaluation points are scaled by a
random constant (the FLINT geometric progression always starts at 1, so
retrying alone would not avoid a leading coefficient vanishing at 1),
and only the leading coefficient of the first argument is required not
to vanish at the evaluation points, the drop in degree of the
specialisations of the second argument being corrected for afterwards.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@vneiger

vneiger commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Nice!

Concerning the evaluation at $c \cdot q^i$, could this maybe be done with several successive extrapolations?
https://flintlib.org/doc/nmod_poly.html#extrapolation
The first batch, done by evaluation, gives you a number batch of evaluations at the first powers of q (and batch is at least the polynomial length), so you could extrapolate with the right offset to get the evaluations at the points batch ... 2*batch-1, then extrapolate the latter to get the evaluations 2*batch ... 3*batch-1, etc. This would at least simplify the code a bit (no need to compute some specific powers, and no need to scale before evaluation), and "in theory" this should not impact the timings negatively but this needs to be checked.

@vneiger

vneiger commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

(note that this adds a dependency that is not wanted in case you plan to introduce multithreading in the evaluation step)

@maelhos

maelhos commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Overall, I made a few tests, and it's about the same performance, though extrapolation is slightly worse in theory (it takes 2L + 2B scalar muls vs. 2L + B for the current version) for L, the total length, and B, the batch length. It also loses the opportunity of parallelization and seems to also be slightly worse for cache / branching. It's still not bad and makes the code easier to read for sure. I'd also argue that since we want to port this code to fmpz_mod and extrapolate is not available for that, we might want to keep it consistent. But overall I'm not opposed to it :)

@maelhos

maelhos commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Guess crediting Vincent and Antoine now breaks the CI... windows is interesting

Adds _gr_poly_resultant_modular / gr_poly_resultant_modular, which computes
the resultant of bivariate polynomials over Z and Q by reducing modulo
word-size primes, calling the geometric multipoint algorithm for each of
them, and reconstructing by Chinese remaindering. _gr_poly_resultant
dispatches to it above a degree cutoff, below which the subresultant PRS
is still faster.

Over Q, denominators are cleared first, using the homogeneity
res(a f, b g) = a^deg_y(g) b^deg_y(f) res(f, g); the same identity divides
out the contents of the inputs in Z and in Z[x] beforehand.

The number of primes is bounded by

  ||res_y(f,g)||_oo <= (sum_i ||f_i||_1^2)^(deg_y(g)/2)
                       (sum_j ||g_j||_1^2)^(deg_y(f)/2)

which is the univariate Hadamard-type bound already used by
fmpz_poly_resultant_modular, applied to the maximum modulus of the
resultant on the unit circle. That bound is only a limit: primes are added
until the reconstruction stops changing, which is detected on a random
linear combination of the coefficients and then confirmed against a fresh
prime. This makes the cost track the actual size of the resultant, which
matters most when it is far below the bound, for instance when the inputs
have a common factor and the resultant vanishes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@vneiger

vneiger commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Overall, I made a few tests, and it's about the same performance, though extrapolation is slightly worse in theory (it takes 2L + 2B scalar muls vs. 2L + B for the current version) for L, the total length, and B, the batch length.

Ok, yes (these are negligible compared to the mulmid, except when the length is really small).

It also loses the opportunity of parallelization

Giving it a second thought, instead of doing the extrapolations with the same offset one after another, we may as well use different offsets B, 2*B, 3*B etc. always from the initial evaluations, and then we can parallelize efficiently.

and seems to also be slightly worse for cache / branching.

Agreed.

It's still not bad and makes the code easier to read for sure. I'd also argue that since we want to port this code to fmpz_mod and extrapolate is not available for that, we might want to keep it consistent. But overall I'm not opposed to it :)

Ok, thanks for having investigated the extrapolation version. I agree that consistency is a plus. There's no clear winner it seems, so, just pick the version you prefer!

For the rest, I'll read the code more carefully in the next few days.

@maelhos

maelhos commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Ok, then I'll keep the current version and finish my prototype of fmpz and fmpq version. Though fmpz_mod will be for another PR as it requires geometric multipoint over fmpz_mod. So far I have very good stats (x486 lgtm) for my prototype over fmpz and fmpq...

$\mathbb Z$:

deg_y deg_x bits subres new speedup notes
4 4 8 4.42e-05 4.44e-05 1.0 below cutoff
4 4 1024 0.00675 0.00703 1.0 below cutoff
4 16 64 0.00257 0.00181 1.4
4 16 1024 0.0682 0.0268 2.5
4 64 8 0.00478 0.00174 2.7
4 64 1024 1.34 0.123 10.9
8 4 8 0.00113 0.000445 2.5
8 4 1024 0.184 0.0421 4.4
8 16 1024 1.22 0.216 5.6
8 64 1024 7.3 0.802 9.1
16 4 64 0.135 0.0195 6.9
16 4 1024 4.17 0.305 13.7
16 16 256 4.53 0.385 11.8
16 64 64 5.08 0.377 13.5
32 4 64 3.11 0.197 15.8
32 16 8 2.3 0.147 15.6
32 64 1024 - 50.2 -

$\mathbb Q$:

deg_y deg_x bits subres new speedup notes
4 4 64 0.0207 0.021 1.0 below cutoff
4 16 8 0.031 0.00277 11.2
4 16 256 11.1 0.425 26.2
4 64 8 9.82 0.0215 456.8
4 64 64 452 0.93 486.0
8 4 64 1.6 0.0383 41.9
8 4 256 21.9 0.304 72.0
8 16 8 0.535 0.0212 25.2
8 64 8 17.4 0.162 107.6

@maelhos

maelhos commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

I'm trying to use FFT-primes for multimodular fmpz to replace geometric multipoint by plain DFT which is sd_fft_trunc and the improvement is very mild... I'll also make the check for nmod version that if prime is FFT we should use sd_fft / fft_small (depending on range) instead of geometric multipoint.

@vneiger maybe we should make some kind of like "whatever multipoint" that depending on the primes either gives dft points (the right one) or geometric or in last resort subproduct tree. This could be usefull for some other things no ?

More generally we could implement this algorithm for generic dense $n$-variate polynomials, we can use Kronecker subsitution to get a complexity of $\mathcal O(d^{2n-1})$

@fredrik-johansson

Copy link
Copy Markdown
Collaborator

Noticed this is a comment:

(currently the Sylvester determinant, since polynomial rings do not report being a UFD)

Normally they do, but e.g. for nmod rings you may have to call gr_ctx_set_is_field to let them know that the modulus is prime. With this the generic dispatcher should already call the subresultant algorithm (or else we need to fix that).

Maël Hostettler and others added 3 commits September 2, 2026 15:44
When the nmod modulus satisfies the fft_small bounds and p - 1 is divisible
by a large enough power of two, _gr_poly_resultant_multipoint evaluates the
coefficients in y at roots of unity with one sd_fft transform each, instead
of the Bluestein product used for a geometric progression. Measured with
alternating rounds on the same prime and inputs, this is 1.10x to 1.26x
faster overall, best at small degrees in y.

A transform produces all its points at once, so this path keeps every value
resident rather than working in blocks; it is used only when they fit a fixed
memory budget, and the blocked geometric evaluation still handles everything
else. The points come out in bit-reversed order, which is undone by gathering
through that permutation, and the geometric interpolation is reused with the
square root of the transform's root of unity as its ratio.

Not used for the Z and Q path: fft_small accepts primes of at most 50 bits,
and needing 1.24x more of them cancels the gain, measured at 0.94x to 1.04x
end to end there, so those keep 62-bit primes and the geometric evaluation.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
nmod, nmod8 and nmod32 answered GR_METHOD_CTX_IS_INTEGRAL_DOMAIN and
GR_METHOD_CTX_IS_FIELD with their primality predicate but left
GR_METHOD_CTX_IS_UNIQUE_FACTORIZATION_DOMAIN unimplemented, so it fell back
to the generic predicate and returned T_UNKNOWN. fmpz_mod, the same situation
of a modulus that may or may not be prime, already answers all three with the
same predicate; Z/nZ is a UFD exactly when n is prime, since otherwise it is
not even a domain.

Anything dispatching on this over nmod, or over a ring built on top of one,
was taking a worse branch. For instance _gr_poly_resultant skipped the
subresultant PRS for bivariate polynomials over nmod and fell through to the
Sylvester determinant, which for degree 8 in y and 8 in x over Z/101Z is 23
times slower.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
With nmod rings now reporting whether they are a UFD, _gr_poly_resultant
reaches the subresultant PRS for bivariate polynomials over nmod instead of
falling through to the Sylvester determinant. The subresultant PRS is faster
than the multipoint algorithm at the smallest sizes, up to about 1.7 times at
degree 3 in y, so the cutoff that was measured against it applies again.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

/* a prime p = m 2^16 + 1, for which the evaluation can use a DFT */
static ulong
_fft_prime(flint_rand_t state, int bits)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there alrady a function that gives fft_primes ?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can have a look at next_fft_number and mpn_ctx_init from fft_small/mpn_mul.c.

The one you write here looks "dangerous" when you call it later in the file with bits potentially as low as 24. Then the first value of m in the loop just below could be negative... well, a ulong so it could be around 64 bits, which I guess is not the intended behaviour.

@maelhos

maelhos commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

A few things:

For the FFT-friendly idea: yes, but no. Basically, for nmod we have a x1.1-x1.2 speedup by using DFT if the prime is FFT.
BUT we are restricted to 50-bit primes, so in practice for multimodular fmpz doing a geometric multipoint with 62-bit primes actually wins; the FFT is ~0.9 times slower on average for input where the coeff in Z are ~100-200 bits.

Normally they do, but e.g. for nmod rings you may have to call gr_ctx_set_is_field to let them know that the modulus is prime. With this the generic dispatcher should already call the subresultant algorithm (or else we need to fix that).

It seems like it wasn't reported as far as I understand; see a97b017
The problem did not seem like it did not know that the modulus was prime but rather was the handler for the UFD function that was missing.

Comment thread doc/source/gr_poly.rst Outdated
@maelhos

maelhos commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

@fredrik-johansson For multiple algebraic attacks, I've used a parallelized version of the resultant algorithm (the inner loop in embarrassingly parallel). I'm not very familiar with how FLINT handles it; the documentation says, "Each version of FLINT brings new functions that are threaded by default," but also examples like "fmpz_mod_mat_mul_classical_threaded" specify it in the name. Should I make a function _gr_poly_resultant_multipoint_threaded (but then it probably won't be dispatched by gr_poly_resultant or should I make it parallel by default?

@vneiger

vneiger commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

@vneiger maybe we should make some kind of like "whatever multipoint" that depending on the primes either gives dft points (the right one) or geometric or in last resort subproduct tree. This could be usefull for some other things no ?

This could surely be useful for other things. But having something like this is not clear to me at all, at the moment. Subproduct trees should probably not be used much except when the reason is that others are not implement, right? And arithmetic progressions may be useful in some cases. DFT points are rarely possible except when we can choose the prime (but then there is no reason to consider other types of points, so the "whatever multipoint" is not that useful). And I'm not sure how easy it will be to make something both general and easy to use (without losing any efficiency)... Maybe this can wait until we have a couple of other relevant situations, to have a better view?

@vneiger

vneiger commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Should I make a function _gr_poly_resultant_multipoint_threaded (but then it probably won't be dispatched by gr_poly_resultant or should I make it parallel by default?

If making it parallel by default does not impact the single-threaded performance, I guess that's ok to make it the default.


/* a prime p = m 2^16 + 1, for which the evaluation can use a DFT */
static ulong
_fft_prime(flint_rand_t state, int bits)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can have a look at next_fft_number and mpn_ctx_init from fft_small/mpn_mul.c.

The one you write here looks "dangerous" when you call it later in the file with bits potentially as low as 24. Then the first value of m in the loop just below could be negative... well, a ulong so it could be around 64 bits, which I guess is not the intended behaviour.

Comment thread src/gr_poly/test/t-resultant_multipoint.c Outdated
Comment thread src/gr_poly/resultant_multipoint.c Outdated
Comment thread src/gr_poly/resultant_multipoint.c Outdated
Comment thread src/gr_poly/resultant_multipoint.c Outdated
Comment thread src/gr_poly/resultant_multipoint.c Outdated
Comment thread src/gr_poly/resultant_multipoint.c Outdated
@vneiger

vneiger commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

To make things deterministic, for methods via geometric progressions, this could rely on:

  • a check that the (prime) field is large enough for the wanted order
  • a method that picks successively the elements 2, 3, 4, 5, ... until the considered element has sufficient order (this should finish after very few attempts if the field is large enough; and this always finishes since this deterministically tries all field elements except 0 and 1).

Similar things were considered in PML and for those two things for the moment we use:

  • A macro NMOD_POLY_CAN_USE_GEOMETRIC(modn, len) which checks that modn is large enough to accommodate geometric progressions (with distinct points) of length len, this also takes some margin to make it fast to find such a progression, see
    https://github.com/vneiger/pml/blob/main/flint-extras/src/nmod_poly_extra.h
  • A function nmod_find_root(n, mod) which deterministically finds an element of order at least n (or detects that none exists), see
    https://github.com/vneiger/pml/blob/main/flint-extras/src/nmod_extra/nmod_find_root.c
    This does not go through finding a root of maximal order, since in most cases n is not that large so this more basic procedure is enough. (Of course if it was easy to add such a maximal order element in the context, this would help for all these geometric progression / FFT things.)

(feel free to re-use these directly if they are helpful)

Comment thread src/gr_poly/resultant_multipoint.c Outdated
Comment thread src/gr_poly/resultant_multipoint.c Outdated
Comment thread src/gr_poly/resultant_multipoint.c Outdated
Comment thread src/gr_poly/resultant_multipoint.c Outdated
maelhos and others added 2 commits September 9, 2026 13:11
Co-authored-by: Vincent Neiger <vneiger@users.noreply.github.com>
@vneiger

vneiger commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Playing a bit with the profile and larger lengths, we can see the impact of the DFT prime case, for some ranges of parameters:

leny  lenx  npoints   subres   multipt  ratio      subres   multipt  ratio
   2  30000    59999   1.90e+00s 1.97e-02s  96.19x   1.88e+00s 2.26e-03s 830.53x
   3  30000   119997   1.96e+00s 6.21e-02s  31.53x   1.91e+00s 9.30e-03s 205.27x
   4  30000   179995   2.10e+00s 2.27e-01s   9.23x   1.95e+00s 1.24e-01s  15.72x
   10  10000   179983   1.49e+00s 5.40e-01s   2.76x   5.96e-01s 3.11e-01s   1.92x
   20  20000   759963   2.64e+01s 4.74e+00s   5.57x   1.37e+01s 2.84e+00s   4.81x

(I took FFT primes containing 2**20 instead of 2**16, to have some room for these large lengths)

So I guess it would make sense to consider (not necessarily within this PR) doing the multi-modular functions using primarily FFT primes as the moduli.

@maelhos

maelhos commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Playing a bit with the profile and larger lengths, we can see the impact of the DFT prime case, for some ranges of parameters:

Oh thats really nice, do we need to tune things to see that happen in practice or should this work as in your test with the curren code ?

maelhos and others added 3 commits September 10, 2026 10:31
Co-authored-by: Vincent Neiger <vneiger@users.noreply.github.com>
Co-authored-by: Vincent Neiger <vneiger@users.noreply.github.com>
@vneiger

vneiger commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

(I see lots of CI failures: I think you applied part of the change but not the main one on the function _sd_fft_get_nmod)

@maelhos

maelhos commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

(I see lots of CI failures: I think you applied part of the change but not the main one on the function _sd_fft_get_nmod)

You're too fast to answer x) Im doing it right now

@vneiger

vneiger commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Playing a bit with the profile and larger lengths, we can see the impact of the DFT prime case, for some ranges of parameters:

Oh thats really nice, do we need to tune things to see that happen in practice or should this work as in your test with the curren code ?

No tuning, I just took the code like it is and in the profile file:

  • modified the parameters lenx, leny in the loops to use those reported in the timings
  • since this is with a large lenx, I modified the FFT prime generation to be sure that it has a root of unity of large enough order (I basically replaced 16 by 20)

@maelhos

maelhos commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

No tuning, I just took the code like it is and in the profile file:

  • modified the parameters lenx, leny in the loops to use those reported in the timings
  • since this is with a large lenx, I modified the FFT prime generation to be sure that it has a root of unity of large enough order (I basically replaced 16 by 20)

Do you think we should integrate tests/profiles in this PR with larger values to better be able to see the FFT performance ?
Maybe we can even but your exact test with 20 bit instead of 16 ?

@vneiger

vneiger commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

No tuning, I just took the code like it is and in the profile file:

  • modified the parameters lenx, leny in the loops to use those reported in the timings
  • since this is with a large lenx, I modified the FFT prime generation to be sure that it has a root of unity of large enough order (I basically replaced 16 by 20)

Do you think we should integrate tests/profiles in this PR with larger values to better be able to see the FFT performance ? Maybe we can even but your exact test with 20 bit instead of 16 ?

Yes, in the profiles having a few of these cases could be useful, to observe this range where DFT becomes useful.

In the test, it's trickier, as this should remain very fast. If adding one case with relatively large depth (e.g. 18) can be done without big impact on the test time, that could be useful. And something else, looking at it I'm not sure that currently the threaded version is tested in the test files?

Comment thread doc/source/gr_poly.rst Outdated
Comment thread doc/source/gr_poly.rst Outdated
@vneiger

vneiger commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Looks good to me, thanks. The last thing that would be needed before merging: that the test suite does cover the case with multiple threads. I think it is not the case currently (?).

@maelhos

maelhos commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

Looks good to me, thanks. The last thing that would be needed before merging: that the test suite does cover the case with multiple threads. I think it is not the case currently (?).

There is a profile for multithread but no "test", do you want to test correctness ?

@vneiger

vneiger commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Looks good to me, thanks. The last thing that would be needed before merging: that the test suite does cover the case with multiple threads. I think it is not the case currently (?).

There is a profile for multithread but no "test", do you want to test correctness ?

Yes, that's it, I guess it would be good that CI tests or make check includes some runs that use the multithreaded variant on a few threads? This would just mean to add a few flint_set_num_threads(1 + n_randint(state, 8)); at the right places in the two new test files. To see other files where that's already done, you can use git grep thread fft_small/test/

maelhos and others added 3 commits September 11, 2026 10:05
Co-authored-by: Vincent Neiger <vneiger@users.noreply.github.com>
Co-authored-by: Vincent Neiger <vneiger@users.noreply.github.com>
@fredrik-johansson

Copy link
Copy Markdown
Collaborator

I think the nmod/fmpz/fmpq poly code doesn't really belong in the gr module. I'd expect to have an actual generic implementation of the multipoint algorithm there.

The nmod_mpoly and fmpz_mpoly modules already have internal dense bivariate types n_bpoly_t and fmpz_bpoly_t, IIRC used for dense bivariate GCDs. Note that these are exactly cast-compatible with gr_poly-of-gr_poly-of-nmod/fmpz coefficients.

So it would make sense to me to move the nmod multipoint code to nmod_mpoly and the fmpz modular algorithm to fmpz_mpoly and base it on these types. (In the future I think I'd like to move the dense bivariate code to nmod_poly/n_poly and fmpz_poly, and the bivariate resultant code would then move along with them, but that can wait.)

Then overload gr_poly_resultant in the method table of the fmpz_poly, fmpq_poly and gr_poly base rings to wrap these.

Then also wrap the bivariate algorithms in nmod_mpoly_resultant, fmpz_mpoly_resultant and fmpq_mpoly_resultant (maybe with appropriate cutoffs if the input is very high-degree/sparse) which are the functions actually used externally right now.

AI review comments:

  • The DFT eligibility checks duplicate _nmod_poly_should_directly_fft in fft_small/plan.c, which is currently static and could be shared.

  • t-resultant_multipoint.c lines 25–26 use 1UL << (bits - 16) with bits up to 50. On 64-bit Windows unsigned long is 32 bits, so this is undefined behaviour. Use UWORD(1).

  • The tests change the thread count with flint_set_num_threads and never restore it, so later tests in the same process inherit up to 8 threads.

  • Commented-out debug // lines are left in.

  • A copy-pasted comment about Sylvester sits in the loop that uses subresultant.

  • git diff --check flags trailing whitespace in the docs, tests and profile files.

  • The result buffers depend on an unstated assumption. They are initialised with a dummy nmod(2) context and later resized under each prime's context. This only works because nmod elements have the same layout for every modulus; at least add a comment saying so.

  • _bivariate_resultant_nmod could fail silently. It truncates the image to outlen without checking. A FLINT_ASSERT(rp->length <= outlen) would catch a bad degree bound.

  • Several paths are never tested:

    • DFT evaluation with geometric interpolation across several blocks. This is the sidx / j >= npoints code. The large test always takes the inverse-transform branch: once npoints ≥ 16384, (ptrunc − npoints)·64 < npoints always holds. The small tests always fit in one block.
    • The FFT-prime branch of the modular algorithm, and its fallback when too few such primes exist. It needs ≥ 2048 points, which no test reaches.
    • Rational denominators, mostly. _randtest_bivariate_zq overwrites every coefficient with an integer. Denominators only appear through the rational content in mode 2, and only on f.

@maelhos

maelhos commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

I think the nmod/fmpz/fmpq poly code doesn't really belong in the gr module. I'd expect to have an actual generic implementation of the multipoint algorithm there.

The nmod_mpoly and fmpz_mpoly modules already have internal dense bivariate types n_bpoly_t and fmpz_bpoly_t, IIRC used for dense bivariate GCDs. Note that these are exactly cast-compatible with gr_poly-of-gr_poly-of-nmod/fmpz coefficients.

So it would make sense to me to move the nmod multipoint code to nmod_mpoly and the fmpz modular algorithm to fmpz_mpoly and base it on these types. (In the future I think I'd like to move the dense bivariate code to nmod_poly/n_poly and fmpz_poly, and the bivariate resultant code would then move along with them, but that can wait.)

Then overload gr_poly_resultant in the method table of the fmpz_poly, fmpq_poly and gr_poly base rings to wrap these.

Then also wrap the bivariate algorithms in nmod_mpoly_resultant, fmpz_mpoly_resultant and fmpq_mpoly_resultant (maybe with appropriate cutoffs if the input is very high-degree/sparse) which are the functions actually used externally right now.

This should be done, not going to lie, I checked how Opus handled this to be sure it didn't mess anything up in the process and it seem ok..

AI review comments:

All the small thing should be settled, I think that Opus also removed some trailing whitespace in other files (probably used a handy command), if that's a problem, let me know.

Also I think this might be a good idea for when we will want to support multipoint resultant over dense multivariate rings (not just bivariate), we can achieve $O(n^{k+1})$ for poly of degree $n$ in $k$ variable by using Kronecker subsitution to reduce to bivariate.

Comment thread .vscode/c_cpp_properties.json Outdated
@maelhos

maelhos commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

Opus also refactored things into fft_small_prime_supports_depth in the fft_small module, it looks identical, though I want to make sure it does not bother other developpement

@vneiger

vneiger commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator

Went over the code again, quickly, and detected no obvious issue with the reorganization. I was just surprised by some things ending up in *_factor modules (but that may be the right place, I just don't know).

@maelhos

maelhos commented Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

Went over the code again, quickly, and detected no obvious issue with the reorganization. I was just surprised by some things ending up in *_factor modules (but that may be the right place, I just don't know).

Well... The bpoly are in *_factor module so there was no way around it.

@vneiger

vneiger commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator

Went over the code again, quickly, and detected no obvious issue with the reorganization. I was just surprised by some things ending up in *_factor modules (but that may be the right place, I just don't know).

Well... The bpoly are in *_factor module so there was no way around it.

Ah, ok. I had seen this for the nmod_mpoly, but had not noticed it was also the case for fmpz_mpoly_factor.

@maelhos

maelhos commented Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

As far as I'm concerned, everything that I wanted to do is done and if everything is good this can be merged. My only concern is with the big restructure requested by @fredrik-johansson but if that is good then should be ok. I also checked performance on the restructured version and it does not make any difference.

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