Opened 9 years ago

Closed 9 years ago

#14833 closed enhancement (fixed)

Make choosing irreducible polynomials independent of finite field implementations

Reported by: Peter Bruin Owned by: Clément Pernet
Priority: major Milestone: sage-5.12
Component: finite rings Keywords: polynomials
Cc: Xavier 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:

Status badges

Description (last modified by Peter Bruin)

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.

Apply: trac_14833-FiniteField_polynomial_choice.patch

Attachments (2)

trac_14833-FiniteField_polynomial_choice.patch (9.7 KB) - added by Peter Bruin 9 years ago.
based on 5.11.beta3 + #14832, use @cached_method
trac_14833_finite_fields_polynomial_choice.patch (205 bytes) - added by Peter Bruin 9 years ago.
empty patch as apéritif for patchbot

Download all attachments as: .zip

Change History (14)

comment:1 Changed 9 years ago by Peter Bruin

Description: modified (diff)
Status: newneeds_review

comment:2 Changed 9 years ago by Xavier Caruso

Cc: Xavier Caruso added

comment:3 Changed 9 years ago by Jean-Pierre Flori

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)".)

comment:4 in reply to:  3 Changed 9 years ago by Peter Bruin

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.

Changed 9 years ago by Peter Bruin

based on 5.11.beta3 + #14832, use @cached_method

comment:5 Changed 9 years ago by Peter Bruin

Description: modified (diff)

comment:6 Changed 9 years ago by Jean-Pierre Flori

Reviewers: Jean-Pierre Flori
Status: needs_reviewpositive_review

Ok, looks ok for me.

comment:7 Changed 9 years ago by Peter Bruin

The "apply" line in the comment below seems to be ignored.

Last edited 9 years ago by Peter Bruin (previous) (diff)

comment:8 Changed 9 years ago by Peter Bruin

For patchbot:

Apply trac_14833-FiniteField_polynomial_choice.patch

comment:9 Changed 9 years ago by Jean-Pierre Flori

Maybe:

Apply: trac_14833-FiniteField_polynomial_choice.patch

That should prevent the Finite Field thing being interpreted by the trac syntax.

Changed 9 years ago by Peter Bruin

empty patch as apéritif for patchbot

comment:10 Changed 9 years ago by Peter Bruin

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.)

Last edited 9 years ago by Peter Bruin (previous) (diff)

comment:11 Changed 9 years ago by Peter Bruin

Apply trac_14833-FiniteField_polynomial_choice.patch

comment:12 Changed 9 years ago by Jeroen Demeyer

Merged in: sage-5.12.beta2
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.