Ticket #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: | Work issues: | ||
| Report Upstream: | N/A | Reviewers: | David Loeffler |
| Authors: | David Roe | Merged in: | sage-5.0.beta10 |
| Dependencies: | Stopgaps: |
Attachments
Change History
comment:2 Changed 15 months 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 15 months ago by davidloeffler
- Status changed from needs_review to needs_work
- Reviewers set to David Loeffler
comment:4 Changed 15 months 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:7 Changed 15 months ago by roed
- Status changed from needs_work to needs_review
I've made the suggested changes, and created #12686 for adding sage.rings.finite_rings to the reference manual.
Changed 15 months ago by davidloeffler
-
attachment
12062-final.patch
added
Apply only this patch. Patch against 5.0.beta8
comment:8 Changed 15 months ago by davidloeffler
- Status changed from needs_review to positive_review
- Description modified (diff)
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:10 Changed 15 months ago by roed
This is a test of http://trac.sagemath.org/experimental
comment:11 Changed 15 months ago by jdemeyer
- Status changed from positive_review to closed
- Resolution set to fixed
- Merged in set to sage-5.0.beta10
