#8547: implement hidden markov models in Cython from scratch (so can remove the
GHMM standard package from Sage)
---------------------------+------------------------------------------------
Reporter: was | Owner: amhou
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-4.4
Component: statistics | Keywords:
Author: | Upstream: N/A
Reviewer: | Merged:
Work_issues: |
---------------------------+------------------------------------------------
Comment(by was):
Replying to [comment:10 jason]:
> Some notes from reading through the code looking at stylistic issues. I
really wish we had
> line-by-line commenting like rietveld for this sort of thing.
>
> 1. Lots of cdef'd functions do not have doctests. I thought the policy
from discussions on sage-devel
> was that *every* function (including cdef functions) should have
doctests (but, of course, these doctests
> would have to either indirectly test the cdef'd function or would have
to test a wrapper of the cdef function).
We definitely do not require doctests for cdef'd methods. The
requirement is that "sage -coverage" returns a score of 100%. This is
clearly stated in the patch reviewers guide. I did seriously consider
them in this case, but literally every single one of these doctests would
just be a literal *exact* copy of a doctest from elsewhere (!) but with #
indirect doctest next to it. There's just no value in that. I could
also make the methods cpdef, but that does incur a performance penalty --
in this case, it would be huge (which is totally unacceptable).
> Some functions (_baum_welch_gamma, for example) don't even have
docstrings.
I do think that all cdef'd methods should have docstrings, and will add
docstrings to any that don't have them (in a part 2 patch).
> 1. I'm curious why IntList.!__getitem!__ does not use the
sage.misc.misc_c.normalize_index function to deal with slices.
> How much of a performance penalty is there?
I'll switch to using normalize_index, since I'm not concerned with
performance for list indexing, since everywhere that performance matters,
I'm directly accessing the memory buffer (which is the only way to really
compete with tightly coded C libraries). So my updated patch will change
to use normalize_index, unless there is a serious problem with doing so.
> Can this "python semantics" part be extracted out to be a more general
> cdef'd counterpart to normalize_index, so that matrices, vectors, and
other types can use it better to implement
> !__getitem!__?
Perhaps. I'm certainly not doing so for this patch.
> Also, as a future enhancement, it doesn't seem that much harder for
!__setitem!__ to also
> support slices. At the very least, the doctests for normalize_index
probably ought to be
> run on the !__getitem!__ function, as they exercise a number of corner
cases for the python semantics.
Fortunately this won't be necessary since I'm switching to it.
> 1. IntList.sum() does not have a doctest for the overflow case
I can add that. I like testing all corner cases.
Thanks for your helpful review of style...
- William
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/8547#comment:11>
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.
