Add resultant methods to fmpz_poly and fmpq_poly #274
Conversation
|
Thanks for this. Can you add a test sort of like this one: python-flint/src/flint/test/test_all.py Lines 3500 to 3501 in b87c681 When possible it is best to have generic tests so that we can be sure that all types implement all methods consistently. The tests can make exceptions for particular types though e.g. it looks like there are no resultant functions for fq polynomials. I suppose that the univariate polynomials have a different signature for resultant because they return a scalar. |
|
@oscarbenjamin tests added! By the way, I discovered that flint's fmpz_mod_poly does not like it when the modulus is composite. I think a comment mentioning this exists in the tests already. I skipped that case since it seems like an upstream problem. However, it is very easy to cause a crash with this behavior: >>> from flint import fmpz_mod_poly, fmpz_mod_poly_ctx
>>> x = fmpz_mod_poly([0, 1], fmpz_mod_poly_ctx(164))
>>> x.resultant(x - 2)
Flint exception (Impossible inverse):
Exception in fmpz_mod_inv: Cannot invert.
Aborted (core dumped)Would it be worth raising exceptions when using fmpz_mod_poly with composite modulus? |
Yes. There are lots of cases like this where FLINT aborts but in python-flint we want to raise an exception instead. For |
|
@oscarbenjamin thanks for the advice! I added an exception for composite moduli in fmpz_mod_poly, and separated that case out in testing. It seems that nmod_poly does not have this problem, so I didn't do anything special for it. |
| if is_field and characteristic == 0: | ||
| # Check that the resultant of two cyclotomic polynomials is right. | ||
| # See Dresden's 2012 "Resultants of Cyclotomic Polynomials" | ||
| for m in range(1, 50): | ||
| for n in range(m + 1, 50): | ||
| a = flint.fmpz_poly.cyclotomic(m) | ||
| b = flint.fmpz_poly.cyclotomic(n) |
There was a problem hiding this comment.
These tests are running in a loop over all polynomial types but don't depend on the type.
I think it would be better just to move all of this out to a separate test_poly_resultants test function but still use _all_polys for the part that loops over the types.
There was a problem hiding this comment.
In general it would be better not to have such large test functions as this one.
There was a problem hiding this comment.
I agree, I'll split the resultant result checking off into a separate test.
In general it would be better not to have such large test functions as this one.
Do you mean test_all_polys is too long, or that this check with cyclotomic polynomials is too long?
There was a problem hiding this comment.
I mean that test_all_polys is too long.
There was a problem hiding this comment.
Ideally the test suite would have reasonably short test functions like test_resultants or something but where each test loops over all possible polynomial types. When a new polynomial type is added it should be possible to just add it to to the list in _all_polys and then update each test or add the methods that are needed for consistency with other types.
| else: | ||
| assert x.resultant(x) == 0 | ||
| assert x.resultant(x**2 + x - x) == 0 | ||
| assert x.resultant(x**10 - x**5 + 1) == S(1) |
There was a problem hiding this comment.
I'm surprised that nmod_poly doesn't crash here. I guess it sometimes does and sometimes doesn't:
In [3]: a = flint.nmod_poly([1, 2, 3], 12)
In [4]: b = flint.nmod_poly([1, 2, 3], 12)
In [5]: a.resultant(b)
Out[5]: 0
In [6]: b = flint.nmod_poly([1, 2, 3, 4], 12)
In [7]: a.resultant(b)
Flint exception (Impossible inverse):
Cannot invert modulo 3*4
Aborted (core dumped)
This adds resultant methods to fmpz_poly (resultant and resultant_modular) and fmpq_poly (resultant), which were previously only exposed for fmpz_mod_poly, fmpq_mpoly, fmpz_mpoly, nmod_mpoly, and fmpz_mod_mpoly.
I included resultant_modular because it's in flint, but in a quick performance test I didn't see much difference between it and fmpz_poly.resultant, or even fmpq_poly.resultant.