Opened 8 years ago
Closed 8 years ago
#14833 closed enhancement (fixed)
Make choosing irreducible polynomials independent of finite field implementations
Reported by: | pbruin | Owned by: | cpernet |
---|---|---|---|
Priority: | major | Milestone: | sage-5.12 |
Component: | finite rings | Keywords: | polynomials |
Cc: | caruso | Merged in: | sage-5.12.beta2 |
Authors: | Peter Bruin | Reviewers: | Jean-Pierre Flori |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #14832 | Stopgaps: |
Description (last modified by )
This patch makes the FiniteField
constructor call the implementation-independent code for choosing irreducible polynomials from #14832.
With this patch, the generic constructor always passes an actual polynomial (not a string specifiying an algorithm to construct one) to the finite field implementation class. For backwards compatibility, the existing classes still recognise a string modulus
. Checking if the finite field is a Conway polynomial is now done by an implementation-independent function.
This is a dependency of #12142.
Attachments (2)
Change History (14)
comment:1 Changed 8 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:2 Changed 8 years ago by
- Cc caruso added
comment:3 follow-up: ↓ 4 Changed 8 years ago by
comment:4 in reply to: ↑ 3 Changed 8 years ago by
Replying to jpflori:
It may be better to use a function decorator to do the caching of the is_conway method, i.e. use @cached_method.
OK, I'll fix this as soon as I have time, unless you or someone else does it first.
And is there any point to prevent setting the default return value for finite fields explicitely constructed with conway polynomials? (i.e. removing what would be the equivalent of "self._is_conway = True" after the "modulus == "conway"" tests? to set the default value of the cached_method you can use "self.is_conway.set_cache(True)".)
That might be slightly more elegant, but the test for modulus == 'conway'
is now in PolynomialRing.irreducible_element
, which is not directly related to finite fields, so we would need an additional check in the finite field constructor. I'm not sure if it is worth it: checking whether we have a Conway polynomial is (hopefully) very fast (since the database was loaded when we looked up the Conway polynomial), and is_conway()
is currently only used for Sage <-> Gap conversion.
comment:5 Changed 8 years ago by
- Description modified (diff)
comment:6 Changed 8 years ago by
- Reviewers set to Jean-Pierre Flori
- Status changed from needs_review to positive_review
Ok, looks ok for me.
comment:7 Changed 8 years ago by
The "apply" line in the comment below seems to be ignored.
comment:8 Changed 8 years ago by
For patchbot:
Apply trac_14833-FiniteField_polynomial_choice.patch
comment:9 Changed 8 years ago by
Maybe:
Apply: trac_14833-FiniteField_polynomial_choice.patch
That should prevent the Finite Field thing being interpreted by the trac syntax.
comment:10 Changed 8 years ago by
Now it thinks it is a digestif, but anyway...
(For the record, that file contained the first version of the patch; when I uploaded a revised version with a different name, the patchbot couldn't be made to eat only the new one.)
comment:11 Changed 8 years ago by
Apply trac_14833-FiniteField_polynomial_choice.patch
comment:12 Changed 8 years ago by
- Merged in set to sage-5.12.beta2
- Resolution set to fixed
- Status changed from positive_review to closed
It may be better to use a function decorator to do the caching of the is_conway method, i.e. use @cached_method.
And is there any point to prevent setting the default return value for finite fields explicitely constructed with conway polynomials? (i.e. removing what would be the equivalent of "self._is_conway = True" after the "modulus == "conway"" tests? to set the default value of the cached_method you can use "self.is_conway.set_cache(True)".)