#1956: implement multivariate truncated power series arithmetic
-------------------------------------------+--------------------------------
Reporter: was | Owner: pernici
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-4.7
Component: commutative algebra | Keywords: multivariate
power series
Work_issues: | Upstream: N/A
Reviewer: Martin Albrecht, Simon King | Author: Niles Johnson
Merged: | Dependencies:
-------------------------------------------+--------------------------------
Changes (by malb):
* status: needs_review => needs_work
Comment:
> 1. Sage passes all doctests with this patch (positive review; buildbot)
I think the buildbot gets confused by the "Apply" instruction in the
ticket's description. Hence, it didn't run the big patch yet.
> 2. All code is documented and doctested thoroughly; documentation builds
without
> error or warning (needs review)
'''needs work'''
{{{
sage -coverage devel/sage/sage/rings/multi_power_series_ring.py
----------------------------------------------------------------------
devel/sage/sage/rings/multi_power_series_ring.py
SCORE devel/sage/sage/rings/multi_power_series_ring.py: 96% (30 of 31)
Missing documentation:
* unpickle_multi_power_series_ring_v0(base_ring, num_gens, names,
order, default_prec, sparse):
}}}
These just need {{{#indirect doctest}}} added:
{{{
sage -coverage devel/sage/sage/rings/multi_power_series_ring_element.py
----------------------------------------------------------------------
devel/sage/sage/rings/multi_power_series_ring_element.py
SCORE devel/sage/sage/rings/multi_power_series_ring_element.py: 100% (52
of 52)
Possibly wrong (function name doesn't occur in doctests):
* _subs_formal(self,*x,**kwds):
* _add_(left, right):
* _sub_(left, right):
* _mul_(left, right):
* _lmul_(self, c):
* _div_(self, denom_r):
----------------------------------------------------------------------
}}}
But the docs build without warnings etc.
> 3. The underlying concept of the implementation (a wrapper for certain
univariate
> power series over multivariate polynomials) is sound (positive review;
Mario
> Pernici)
> 4. multi_power_series_ring.py: the code accurately does what it claims
to do (needs_review)
Shouldn't {{{def _element_constructor_(self,f)}}} use the default
precision of the ring?
> 5. multi_power_series_ring_element.py: the code accurately does what it
claims to
> do (needs review)
> 6. Integration with the rest of sage: construction and use of
PowerSeriesRings
> works correctly, and parallels behavior of polynomial rings (needs
review)
'''needs work'''
The {{{default_prec}}} parameter does not seem to work as expected (or is
my expectation wrong?)
{{{
#!python
sage: P.<x,y> = PowerSeriesRing(GF(127),default_prec=20)
sage: x.prec()
+Infinity
}}}
> 7. Performance: the multivariate power series arithmetic is fast enough
to be
> included in Sage (positive review; Mario Pernici)
> 8. Coding: the code is free from obvious inefficiencies in error
handling, memory
> management, etc. (needs review)
If the performance is good (7), then I think we can just assume it's okay.
We can always come back and fix stuff later which is too slow.
I have to say that I don't like the excessive use of double underscore
attributes. It makes it harder to inherit from it or reach into the
internals (just as you have to write
{{{_PowerSeriesRing_generic__poly_ring}}}) I'd just use a single
underscore. I'll leave it up to you (i.e., not make my review dependent on
it) whether you insist on keeping them.
> 9. The items on this list constitute a complete review (needs review)
Fine by me.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/1956#comment:105>
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.
