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:

Status badges

Description (last modified by pbruin)

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 pbruin 8 years ago.
based on 5.11.beta3 + #14832, use @cached_method
trac_14833_finite_fields_polynomial_choice.patch (205 bytes) - added by pbruin 8 years ago.
empty patch as apéritif for patchbot

Download all attachments as: .zip

Change History (14)

comment:1 Changed 8 years ago by pbruin

  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 8 years ago by caruso

  • Cc caruso added

comment:3 follow-up: Changed 8 years ago by jpflori

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 8 years ago by pbruin

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 8 years ago by pbruin

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

comment:5 Changed 8 years ago by pbruin

  • Description modified (diff)

comment:6 Changed 8 years ago by jpflori

  • 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 pbruin

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

Last edited 8 years ago by pbruin (previous) (diff)

comment:8 Changed 8 years ago by pbruin

For patchbot:

Apply trac_14833-FiniteField_polynomial_choice.patch

comment:9 Changed 8 years ago by jpflori

Maybe:

Apply: trac_14833-FiniteField_polynomial_choice.patch

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

Changed 8 years ago by pbruin

empty patch as apéritif for patchbot

comment:10 Changed 8 years ago by pbruin

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 8 years ago by pbruin (previous) (diff)

comment:11 Changed 8 years ago by pbruin

Apply trac_14833-FiniteField_polynomial_choice.patch

comment:12 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.12.beta2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.