#1956: implement multivariate power series arithmetic
-----------------------------------+----------------------------------------
Reporter: was | Owner: malb
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-5.0
Component: commutative algebra | Keywords: multivariate power series
Author: niles | Upstream: N/A
Reviewer: | Merged:
Work_issues: |
-----------------------------------+----------------------------------------
Changes (by SimonKing):
* keywords: => multivariate power series
* milestone: sage-feature => sage-5.0
Comment:
First of all: It would certainly be good to have multivariate power series
in Sage. Also the performance seems to be pretty good. Thank you!
I have some criticism, though.
1. {{{__contains__}}} and coercion
I was reading part of the patch, and I see that your multivariate power
series rings have a custom {{{__contains__}}} method. I think it should be
possible to simply inherit it.
Moreover, the custom method seems to provide a non-standard behaviour:
Usual in Sage is to have {{{a in R}}} if and only if {{{a==R(a)}}} is True
(of course, if {{{R(a)}}} raises an error then a is not in R).
Another formulation of the rule is: If a.parent() is P and R has a
coercion map from P then a is in R: Since there is a coercion map, R(a) is
supposed to work, and {{{a==R(a)}}} is equivalent to {{{R(a)==R(a)}}},
because this is how comparison with the coercion model works.
But you have in your doctests
{{{
sage: a in R
False
sage: R(a) in R
True
}}}
in a situation were apparently a coercion from {{{a.parent()}}} to {{{R}}}
exists.
2. Use of double-underscore methods
In your {{{__cmp__}}} method, you do
{{{self._bg_value.__cmp__(other._bg_value)}}}
Generally, one should avoid to call double-underscore methods directly.
So, better do
{{{cmp(self._bg_value, other._bg_value)}}}
The same applies to doctests. So, instead of
{{{
sage: R.<x,y> = MPowerSeriesRing(GF(17))
sage: R
Multivariate Power Series Ring in x, y over Finite Field of size 17
sage: R.__repr__()
'Multivariate Power Series Ring in x, y over Finite Field of size 17'
}}}
you should do
{{{
sage: R # indirect doctest
Multivariate Power Series Ring in x, y over Finite Field of size 17
}}}
and instead of
{{{
sage: M = MPowerSeriesRing(ZZ,5,'t');
sage: N = M.remove_var(M.gens()[2])
sage: M.__contains__(N.random_element(10))
False
sage: M.__contains__(M.random_element(10))
True
}}}
you should have
{{{
sage: M.random_element(10) in M
True
}}}
I think it should even be
{{{
sage: N.random_element(10) in M
True # not False!
}}}
because, if I understand correctly, there is a coercion from N to M --
see point 1.
I am now running {{{make ptestlong}}}, and will then have a closer look at
the code - so, no review yet. But I think you should address the points
above.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/1956#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.
