#9706: New Version of orthogonal Polynomials
-------------------------------+--------------------------------------------
Reporter: maldun | Owner: burcin, maldun
Type: enhancement | Status: needs_work
Priority: minor | Milestone: sage-5.0
Component: symbolics | Keywords: orthogonal polynomials, symbolics
Author: Stefan Reiterer | Upstream: N/A
Reviewer: Burcin Erocal | Merged:
Work_issues: |
-------------------------------+--------------------------------------------
Changes (by burcin):
* cc: fstan (added)
* reviewer: => Burcin Erocal
* status: needs_review => needs_work
Comment:
Great work Stefan. Your patch looks good overall, but it needs a lot of
polish. Thank you very much for this.
Here are some quick comments after reading
attachment:trac_9706_orthogonal_polys.patch. I didn't try to apply and run
the code yet. It would be better if other people try this as well since I
am really short on time these days.
* I suggest you use your real name in the HG headers. This information is
used for copyright/license issues as well. In the future it might cause a
lot of trouble if people have to chase down `maldun` for copyright
questions.
* You shouldn't import any part of `numpy` at the module level. This
slows down startup too much. See #3561 for example. I'd say the same holds
for `mpmath` and `scipy`.
* line 385-386 has this:
{{{
Then after using one of these functions, it changes:: (The value is now
False for chebyshev_T because chebyshev_T uses clenshaw method instead...)
}}}
I don't think this is valid Sphinx.
* delete line 412
{{{
#load /home/maldun/sage/sage-4.5.2/devel/sage-
ortho/sage/functions/orthogonal_polys.py
}}}
* line 419: he -> the
* There are no doctests for the `OrthogonalPolynomial` class, make sure
your file passes `sage -coverage`
* The commented timings in the docstring of
`OrthogonalPolynomial._clenshaw_method_()` are confusing. It would be
better if you provide a function in the same file that does these timings
automatically and prints out the results. You should at least delete this
from the documentation though.
* In the docstring of `OrthogonalPolynomial._eval_()`
* remove the empty first line (line 494) of
* remove the commented out timings as well
* you need an empty line after `EXAMPLES::`
* the empty last line should be removed
* add some comments to the `OrthogonalPolynomial._eval_()` method to
indicate what you're trying to do with these tests.
* lines 583-593 have a confusing comment and a bug
{{{
try:
#s = maxima(self._maxima_init_evaled_(*args))
#This above is very inefficient! The older
#methods were much faster...
return self._maxima_init_evaled_(*args)
except TypeError:
return None
if self._maxima_name in repr(s):
return None
else:
return s.sage()
}}}
* You don't need to state "Class for" on line 598, "The Chebyshev ..." is
enough.
* Why do you delete the `chebyshev_T(2,x)` test on line 371? You can just
add the new ones after that.
* line 626, `EXAMPLES:` -> `EXAMPLES::`
* Don't use `*args` or `**kwds` when you don't need them. Name the
arguments and be explicit. Remember the "Zen of Python", "Explicit is
better than implicit."
* OK, generally, fix the docstrings to conform to Sphinx standards. This
should be documented somewhere in the developers guide.
* line 673, `_maxima_init_evaled_()` doesn't have doctests.
* line 678 - , `_clenshaw_method_()`
* docstring is not indented properly.
* It would be better to put the recursion formula in the docstring.
* line 790 `_clenshaw_method_()` doesn't have doctests.
* There is something wrong with the `_maxima_init_evaled_()` on line 821.
Are you sure this function shouldn't just return a string to be run in
maxima? How do we know that doctest actually calls this function? In any
case, the right way to convert a maxima object to sage is to run `.sage()`
on it. Never use `sage_eval()` on a string in the Sage library.
* Calls to mpmath should be able to use the precision directly from the
type of the argument now. Are you sure all this is necessary:
{{{
try:
step_parent = kwds['parent']
except KeyError:
step_parent = parent(args[-1])
try:
precision = step_parent.prec()
except AttributeError:
precision = RR.prec()
}}}
See #9566.
* line 924, change the error message to something more professional.
"Derivative w.r.t. to the index is not supported, yet, and perhaps never
will be..." is not acceptable. "Derivatives with respect to the index is
not supported." would be enough.
* Document the derivative formula in the docstring, using proper math
notation
* What needs to be discussed from the comments on line 968-974?
* Same for lines 1058-1060?
* no doctests for `_clenshaw_method_()` on line 1156.
* no doctests for `_maxima_init_evaled_()` on line 1189.
I give up at this point. It seems that there are similar issues in the
rest of the file as well.
After you clean up the code according to the comments above, perhaps a
native English speaker like Karl-Dieter or Minh can help with the
documentation.
Thanks again for all your work.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/9706#comment:32>
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.
