#6781: [with patch, needs review] Library access to ecl
---------------------------+------------------------------------------------
Reporter: nbruin | Owner: nbruin
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-4.3.3
Component: packages | Keywords:
Author: Nils Bruin | Upstream: N/A
Reviewer: | Merged:
Work_issues: |
---------------------------+------------------------------------------------
Comment(by nbruin):
Thanks for your feedback! It'll be nice to get this in.
> * The module name you declare on line 431 of `module_list.py` should be
sage.libs.ecl.ecl.
Are you sure? the current thing seems to work too and I like not typing
the extra 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.
Sure. However, I just picked and chose from the ecl headers, and I
orthogonalized some of the names. Wouldn't moving those to decl.pxd raise
the false impression that the declarations there are a literal translation
of ecl.h ?
> * In `ecl.pyx`, I'm not sure if you need both of these lines
> {{{
> cimport sage.rings.integer
> from sage.rings.integer cimport Integer
> }}}
I guess we can try without, but I think I put it in because it didn't work
without.
> * line 280 in `ecl.pyx` should be `elif pyobj is None:`. Testing with
`is` is much faster than a comparison.
Good one!
> * 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.
You have a good editor. Mine doesn't do that.
> * In `ecl.pyx`, line 653 - 664 are redundant.
But they don't hurt performance either do they? I like them for
documentation purposes, so that I know the codes that go into a rich_cmp
> * 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.
Do you think any time spent on tuning those tests is well-spent? They're
really just there because they need some test. If they fail, nothing in
ecl will work.
> * It's more pythonic to use `is_character()`, `is_null()`, `is_list()`,
etc. instead of `characterp()`, `nullp()`, `listp()`, ...
but characters not being strings is not very pythonic either. We're
interfacing with a different language, so I think it makes sense to depart
from python's naming conventions. Also, CL routines are standard and well-
documented. If we use those names, we get a well-defined interface without
thinking. If we start changing function names for wrappers, we need to
start thinking and may screw up.
> * You can replace line 996-997 of `ecl.pyx` with the following:
> {{{
> cdef EclObject obj = <EclObject>PY_NEW(EclObject)
> }}}
Great!
I'll see if I can update the patch once you've had some run time with it
and commented on its functionality.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/6781#comment:20>
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.
