#6781: [with patch, needs review] Library access to ecl
---------------------------+------------------------------------------------
Reporter: nbruin | Owner: nbruin
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-4.3.2
Component: packages | Keywords:
Author: Nils Bruin | Upstream: N/A
Reviewer: | Merged:
Work_issues: |
---------------------------+------------------------------------------------
Comment(by burcin):
Hi Nils,
Sorry for not looking at this before. I just read the patch and it looks
really good. I'd like to try to get this in the next release if possible,
to prepare for #7377 proper.
A few quick comments after reading the patch (didn't apply or build yet,
maybe over the weekend):
* The module name you declare on line 431 of `module_list.py` should be
sage.libs.ecl.ecl.
* We would normally put the declarations imported from a header file in a
`decl.pxd` in `sage/libs/ecl/`. I suggest renaming the current `ecl.pxd`
to `decl.pxd`, and using `from sage.libs.ecl.decl cimport ...` in
`ecl.pyx` to import the declarations you need. Then `ecl.pxd` can be used
to declare the Cython methods/classes from `ecl.pyx` which should be
available for imports from other modules.
* In `ecl.pyx`, I'm not sure if you need both of these lines
{{{
cimport sage.rings.integer
from sage.rings.integer cimport Integer
}}}
* line 280 in `ecl.pyx` should be `elif pyobj is None:`. Testing with
`is` is much faster than a comparison.
* Including a TODO list at the beginning could be helpful. For example
you can note that optimizing conversion of gmp integers and rationals
needs work. My editor highlights "TODO" so you can also just put that as a
part of the comment in the `python_to_ecl()` function.
* In `ecl.pyx`, line 653 - 664 are redundant.
* The doctests for `car()`, `cdr()`, `caar()`, etc. are very instructive,
but they are all the same. If anything goes wrong we'll get the same error
from all the functions, not knowing where to look. Since this is not going
to be exposed much to the users, I suggest putting the instructive example
somewhere along the top of the file, and including a test only for the
corresponding function in the docstring.
* It's more pythonic to use `is_character()`, `is_null()`, `is_list()`,
etc. instead of `characterp()`, `nullp()`, `listp()`, ...
* You can replace line 996-997 of `ecl.pyx` with the following:
{{{
cdef EclObject obj = <EclObject>PY_NEW(EclObject)
}}}
This will avoid the python class initialization overhead and make things
slightly faster.
Thanks a lot for your work.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/6781#comment:19>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
