#6781: Library access to ecl
--------------------------------------------------+-------------------------
Reporter: nbruin | Owner: nbruin
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-4.3.4
Component: packages | Keywords:
Author: Nils Bruin | Upstream: N/A
Reviewer: Burcin Erocal, Karl-Dieter Crisman | Merged:
Work_issues: |
--------------------------------------------------+-------------------------
Changes (by burcin):
* status: needs_info => needs_work
* reviewer: => Burcin Erocal, Karl-Dieter Crisman
Comment:
Replying to [comment:20 nbruin]:
> Thanks for your feedback! It'll be nice to get this in.
I'm sorry I took so long to respond. At least I managed to test your
patch(es, with #7377) in the meanwhile.
I'm generally happy with how they look / work and I'd like to get them
merged as soon as possible (I know I'm the main cause of delays). Feel
free to change this ticket to positive review once the minor issues below
are resolved.
> > * 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.
The convention is to make the module name correspond to the file name. If
you think a single file will be enough, then you can just remove the
directory `ecl` and put the `ecl.p*` files under `sage/libs/` directly.
I'm afraid I have to insist on one of these options.
> > * 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 ?
It's common to change the types or the function names a little. This is a
minor issue though, I just think the code will be easier to maintain that
way.
> > * 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.
OK.
> > * 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.
I just use `vim`. :)
>
> > * 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
Why don't you just put them in comments? Looking at the code, especially
if you're scrolling up, it's not immediately clear that those lines are
never used. If they are only for information, they should be comments.
> > * 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.
OK.
> > * 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.
OK.
> > * You can replace line 996-997 of `ecl.pyx` with the following:
> > {{{
> > cdef EclObject obj = <EclObject>PY_NEW(EclObject)
> > }}}
>
> Great!
I see that you used `EclObject.__new__()` in your last patch attached to
#7377. How does that compare with using `PY_NEW()` in terms of
performance?
AFAICT, some of the changes I suggest above are available in the last
patch attached to #7377. Can you submit a patch with only that changes
above to this ticket? Then we can get this ticket merged without waiting
to solve the problems with the other one. If you don't have time, I can
also make a patch with these changes (hopefully in reasonable time).
Many thanks for this interface again!
BTW, I don't see why this needs to be tested on Solaris. (In any case,
expecting every developer to test their patches on solaris is
unreasonable, IMHO.)
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/6781#comment:22>
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.
