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 )
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)
Change History (9)
comment:1 Changed 8 years ago by
- Owner changed from AlexGhitza to lftabera
comment:2 Changed 8 years ago by
- Status changed from new to needs_review
comment:4 Changed 8 years ago by
- 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
comment:5 follow-up: ↓ 6 Changed 8 years ago by
- 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
- 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
- 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
- Merged in set to sage-4.8.alpha2
- Resolution set to fixed
- Status changed from positive_review to closed
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.