Opened 8 years ago

Closed 8 years ago

#11784 closed defect (fixed)

Add more sanity checks to FiniteField constructor

Reported by: lftabera Owned by: lftabera
Priority: major Milestone: sage-4.8
Component: basic arithmetic Keywords: beginner GF
Cc: Merged in: sage-4.8.alpha2
Authors: Luis Felipe Tabera Alonso Reviewers: Johan Bosman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

Galois (i.e. finite) fields admits a modulus argument and by default it checks if the modulus is irreducible mod the characteristic of the field.

However, it does not check that the polynomial is of the right degree.

An example:

sage: K=GF(3**2,name='a', modulus=QQ[x](x^3-x+1))
sage: K
Finite Field in a of size 3^2
sage: K.list()
[0, a + 1, 1, 2, 1, 2, 1, 2, 1]
sage: K.modulus()
x^2

Attachments (1)

trac-11784-Galois_fields.patch (5.6 KB) - added by lftabera 8 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 8 years ago by lftabera

  • Owner changed from AlexGhitza to lftabera

I have decided that if check_irreducible = False, then the degree of the polynomial is not checked. It is assumed that check_irreducible is there for users that know what they are doing.

comment:2 Changed 8 years ago by lftabera

  • Authors set to Luis Felipe Tabera Alonso
  • Status changed from new to needs_review

comment:3 Changed 8 years ago by lftabera

  • Description modified (diff)

Rebase agains 4.7.2

comment:4 Changed 8 years ago by johanbosman

  • Keywords beginner added; begginer removed
  • Status changed from needs_review to needs_work

It surprises me that Sage didn't check this, wow.

At a few points, your spacing in the docstring is not in compliance with the Python guideline (see http://www.python.org/dev/peps/pep-0008/). In order to avoid messiness or sloppiness in the documentation, it would be good to correct this.

For instance:

irreducibility modulo p. Also, the modulus has to be of the right degree.

The period should be followed by two spaces.

sage: L=GF(3**2,name='a', modulus=QQ[x](x-1), check_irreducible=False) 
sage: L.list() #random 

In the first line, there should be spaces between the = operator and a space after the first comma. In the second line, put 2 spaces before # and 1 after #.

It would also be good to put spaces around a few more mathematical operators. Though I don't think we should be as strict as in the Python guideline here; just do it in cases where it will increase readability (don't do it for for instance).

Changed 8 years ago by lftabera

comment:5 follow-up: Changed 8 years ago by lftabera

  • Status changed from needs_work to needs_review

Thanks for realizing this.

I have updated the patch according to the python guidelines. The only one I have not followed is the limit of 79 characters that is surpassed in some exception messages and is not followed for similar code along the Sage library. But I can also change this.

I have changed the exception style to Exception("message") style and added a dot in Exception messages.

comment:6 in reply to: ↑ 5 Changed 8 years ago by johanbosman

  • Reviewers set to Johan Bosman
  • Status changed from needs_review to positive_review

Replying to lftabera:

Thanks for realizing this.

I have updated the patch according to the python guidelines. The only one I have not followed is the limit of 79 characters that is surpassed in some exception messages and is not followed for similar code along the Sage library. But I can also change this.

It would indeed be better, but I think it is okay like this.

I have changed the exception style to Exception("message") style and added a dot in Exception messages.

This is probably much better than Exception, "message" indeed, which everyone uses. :).

The patch looks good now, works, and passes all tests.

comment:7 Changed 8 years ago by jdemeyer

  • Description modified (diff)
  • Keywords GF added
  • Summary changed from Add more sanity checks to Galois Field constructor to Add more sanity checks to FiniteField constructor

comment:8 Changed 8 years ago by jdemeyer

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