fix: irreducibility of constant polynomials over finite fields #1988
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1988 +/- ##
==========================================
+ Coverage 87.98% 88.01% +0.02%
==========================================
Files 98 98
Lines 36063 36031 -32
==========================================
- Hits 31730 31711 -19
+ Misses 4333 4320 -13 ☔ View full report in Codecov by Sentry. |
src/flint/fq_default_poly.jl
Outdated
| is_zero(x) && return false | ||
| is_unit(x) && return false |
There was a problem hiding this comment.
How about this (at least when the coefficients are in a field) ?
| is_zero(x) && return false | |
| is_unit(x) && return false | |
| is_constant(x) && return false |
| @@ -501,6 +501,9 @@ end | |||
| @test is_irreducible(x) | |||
|
|
|||
| @test is_irreducible(x^16+2*x^9+x^8+x^2+x+1) | |||
|
|
|||
| @test !is_irreducible(x^0) | |||
| @test !is_irreducible(0*x^0) | |||
There was a problem hiding this comment.
I think it would be better to add this kind of tests to the conformance test suits for polynomial rings instead. Of course we can also test it here first, no big harm (though I think at some point we should get somebody to go through the Nemo test files and weed out tests that are completely covered by the generic conformance tests)
There was a problem hiding this comment.
Sure, it can be added it to the conformance tests. But still I have the impression that the conformance tests are nice to have, but they are a bit of a black box and it is not clear to me what they guarantee to test. This here fixes a specific bug of the various Nemo methods, so I don't see why this should not be tested explicitly here.
There was a problem hiding this comment.
Yeah, to know what the conformance tests test, you need to look into them. Like so many things in AA/Nemo/Oscar that are unfortunately underdocumented :-(. At least they are relatively small, contained in a single file, and it is easy to look in there and see what is tested.
But I am not opposed to adding the tests here, I just think we should add them to the conformance tests, too. Sadly I forgot to do so when I fixed the bug here for QQMPolyRingElem in December, because then we'd have found & fixed the others fixed here a month earlier :-).
Anyway, I've added such tests now in Nemocas/AbstractAlgebra.jl#1952 (and of course that PR fails for now, until this PR here is merged :-) )
|
I adjusted it to use |
Flint currently considers all constant polynomials irreducible, which is a bit questionable. This PR here makes these methods follow our convention, see
(and the corresponding generic method.)
Whether this is a bug upstream is to be decided, see flintlib/flint#2140.