Opened 10 years ago

Closed 10 years ago

# Add more sanity checks to FiniteField constructor

Reported by: Owned by: lftabera lftabera major sage-4.8 basic arithmetic beginner GF sage-4.8.alpha2 Luis Felipe Tabera Alonso Johan Bosman N/A

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
```

### comment:1 Changed 10 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 10 years ago by lftabera

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

### comment:3 Changed 10 years ago by lftabera

• Description modified (diff)

Rebase agains 4.7.2

### comment:4 Changed 10 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).

### comment:5 follow-up: ↓ 6 Changed 10 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 10 years ago by johanbosman

• Reviewers set to Johan Bosman
• Status changed from needs_review to positive_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.

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 10 years ago by jdemeyer

• Description modified (diff)