sage.rings.finite_rings: Modularization fixes, # needs
#36056
Conversation
…ntl' when GF2X is used
…ings.finite_rings --only-tags src/sage/rings/finite_rings
…r sage.databases.conway
…integer_mod, sage.rings.factorint
| @@ -45,19 +48,19 @@ def conway_polynomial(p, n): | |||
|
|
|||
| EXAMPLES:: | |||
|
|
|||
| sage: conway_polynomial(2,5) | |||
| sage: conway_polynomial(2,5) # needs conway_polynomials | |||
| x^5 + x^2 + 1 | |||
There was a problem hiding this comment.
Shouldn't the tag be file-scoped?
There was a problem hiding this comment.
This module also contains code for computing pseudo-Conway polynomials, which has to be used when the database is not available
| @@ -1,4 +1,4 @@ | |||
| # sage.doctest: optional - sage.rings.finite_rings | |||
| # sage.doctest: needs sage.rings.finite_rings | |||
| """ | |||
There was a problem hiding this comment.
Aren't all files in src/sage/rings/finite_rings installed by the distribution that provides the feature?
There was a problem hiding this comment.
No, sagemath-categories ships the basic modules of sage.rings.finite_rings - https://github.com/mkoeppe/sage/blob/t/32432/modularization_of_sagelib__break_out_a_separate_package_sagemath_polyhedra/pkgs/sagemath-categories/MANIFEST.in.m4#L109-L123
This makes the prime fields and the integer mod rings available.
| try: | ||
| return ConwayPolynomials().has_polynomial(p,n) | ||
| except ImportError: | ||
| return False |
There was a problem hiding this comment.
Isn't it incorrect to return False when the database is not available? Why catch ImportError?
There was a problem hiding this comment.
The method is named exists_conway_polynomial, but it actually only tests whether it is known to Sage. When the database is not installed, nothing is known to Sage.
Return False here triggers the use of pseudo-Conway polynomials in the relevant code paths.
There was a problem hiding this comment.
I see. That is reasonable. Thanks.
| Partially defined reduction map: | ||
| From: Fraction Field of | ||
| Univariate Polynomial Ring in t over Finite Field of size 2 (using GF2X) | ||
| To: Residue field in tbar of Principal ideal (t^7 + t^6 + t^5 + t^4 + 1) of | ||
| Univariate Polynomial Ring in t over Finite Field of size 2 (using GF2X) | ||
| sage: type(k) | ||
| sage: type(k) # needs sage.libs.linbox | ||
| <class 'sage.rings.finite_rings.residue_field_givaro.ResidueFiniteField_givaro_with_category'> |
There was a problem hiding this comment.
Is this right? Needs sage.libs.linbox to get type(k) but does not to create k?
There was a problem hiding this comment.
There are multiple implementations available that can cover a given p.
This example works even when NTL and LinBox are not available, but the tests hardcode the implementation ("using GF2X") based on NTL and LinBox being standard packages.
There was a problem hiding this comment.
Ah, I see. Thanks for explaining.
There was a problem hiding this comment.
But we may handle this in a similar way as below.
There was a problem hiding this comment.
I've made the change below because that doctest is just displaying the codomain.
Here k has already been displayed, so the doctest seems to be intended to display a specific type.
So I'm keeping it in the current form
| sage: R.<t> = GF(2)[]; h = t^5 + t^2 + 1 | ||
| sage: k.<a> = R.residue_field(h) | ||
| sage: K = R.fraction_field() | ||
| sage: L = k.lift_map(); L.codomain() | ||
| sage: L = k.lift_map(); L.codomain() # needs sage.libs.ntl | ||
| Univariate Polynomial Ring in t over Finite Field of size 2 (using GF2X) |
There was a problem hiding this comment.
If sage.libs.ntl is needed just for k.lift_map() and L.codomain(), perhaps sage.rings.finite_rings should depend on sage.libs.ntl. No?
There was a problem hiding this comment.
No, the test also works when only PARI provides an implementation, but then the text "(using GF2X)" is missing.
There was a problem hiding this comment.
I see.
An alternative (but less confusing) way to handle it is
sage: L = k.lift_map(); L.codomain()
Univariate Polynomial Ring in t over Finite Field of size 2 (...)
or something similar.
There was a problem hiding this comment.
OK, done in 744ddf8.
|
Documentation preview for this PR (built with commit 744ddf8; changes) is ready! 🎉 |
|
Thank you! |
📝 Checklist
⌛ Dependencies