Opened 11 years ago

Closed 11 years ago

#12062 closed enhancement (fixed)

FiniteField_ntl_gf2e to Python

Reported by: roed Owned by: AlexGhitza
Priority: major Milestone: sage-5.0
Component: algebra Keywords: finite fields
Cc: Merged in: sage-5.0.beta10
Authors: David Roe Reviewers: David Loeffler
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

GitHub link to the corresponding issue

Description (last modified by davidloeffler)

In order to create dynamic classes with finite fields and use the category system most effectively, parents should be in Python and not Cython.

Prerequisite for: #12064, #12262, #12077, #8240.

Apply 12062-final.patch

Attachments (3)

12062.patch (56.9 KB) - added by roed 11 years ago.
12062_ref_changes.patch (5.9 KB) - added by roed 11 years ago.
12062-final.patch (58.9 KB) - added by davidloeffler 11 years ago.
Apply only this patch. Patch against 5.0.beta8

Download all attachments as: .zip

Change History (14)

comment:1 Changed 11 years ago by roed

Status: newneeds_review

Changed 11 years ago by roed

Attachment: 12062.patch added

comment:2 Changed 11 years ago by davidloeffler

This is looks great, but here are some (relatively minor) gripes:

  • In the changes to integer.pyx, the variable "broken" is assigned to but never used.
  • The patchbot is grumbling about some lines with trailing whitespace.
  • There are some TeX formulae marked with $ signs rather than backticks.
  • The docstring for fetch_int in the cython file has been rewritten and is now much more comprehensible, but the docstring for the same function in the new python file is still the old rather confusing one.
  • Given that you've put in the work to Sphinxify the docstrings, maybe we should add the new python file finite_field_ntl_gf2e.py to the reference manual? (This might be better off on a new ticket, since it makes no sense to have some backends and not others in the reference manual, and doing the others will be lots of work.)
  • A brief docstring for cdef class Cache_ntl_gf2e(SageObject): would be nice, explaining briefly why the class exists.

comment:3 Changed 11 years ago by davidloeffler

Reviewers: David Loeffler
Status: needs_reviewneeds_work

comment:4 Changed 11 years ago by roed

Thanks for all the reviews David! I'm at the Fields Institute this week for a conference, but will try to find some time to make changes.

comment:5 Changed 11 years ago by davidloeffler

Description: modified (diff)

comment:6 Changed 11 years ago by davidloeffler

Description: modified (diff)

Changed 11 years ago by roed

Attachment: 12062_ref_changes.patch added

comment:7 Changed 11 years ago by roed

Status: needs_workneeds_review

I've made the suggested changes, and created #12686 for adding sage.rings.finite_rings to the reference manual.

Changed 11 years ago by davidloeffler

Attachment: 12062-final.patch added

Apply only this patch. Patch against 5.0.beta8

comment:8 Changed 11 years ago by davidloeffler

Description: modified (diff)
Status: needs_reviewpositive_review

Hi David,

I'm happy with this: you've dealt with all of my concerns. Positive review. (Your second patch didn't have a commit message, so I qfolded it into the previous one, which also means we don't clutter the repository history unnecessarily; I didn't actually change anything in the code.)

comment:9 Changed 11 years ago by roed

Thanks!

comment:11 Changed 11 years ago by jdemeyer

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