#13215: Skew polynomials
-------------------------------------+-------------------------------------
Reporter: caruso | Owner: tbd
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-7.3
Component: algebra | Resolution:
Keywords: skew polynomials | Merged in:
Authors: Xavier Caruso | Reviewers: Burcin Erocal, David
| Lucas
Report Upstream: N/A | Work issues:
Branch: | Commit:
u/arpitdm/skew_polynomials | 0de3552ca85ee36a91cb41bff993e0eaebe25ecb
Dependencies: #13214, #13303, | Stopgaps:
#13640, #13641, #13642 |
-------------------------------------+-------------------------------------
Changes (by tscrim):
* status: needs_review => needs_work
Comment:
The important thing is the first comment. The rest is more polish, but I
think a fair amount more is needed.
- ''All'' methods and functions must have doctests (e.g., the one added to
`map.pyx` and hidden methods). Also, I think you should remove the
commented out code.
- You should remove this line from `module_list.py`:
{{{
# depends = numpy_depends),
}}}
- I would swap these lines in `rings.py`:
{{{#!diff
- from sage.categories.morphism import Morphism
if isinstance(arg, tuple):
+ from sage.categories.morphism import Morphism
if len(arg) == 2 and isinstance(arg[1], Morphism):
from
sage.rings.polynomial.skew_polynomial_ring_constructor import
SkewPolynomialRing
return SkewPolynomialRing(self, arg[1], names=arg[0])
}}}
- Remove all trailing whitespace (well, at least do not add any).
- In `skew_polynomial_element.pyx`, there are a number of things that need
to be in latex mode (and an equation using the `.. MATH::` block).
- I would move most-to-all of the module-level doc to the user entry point
and then have a `.. SEEALSO:: block referencing said doc`, so it is
visible from the `?` on the command line. This also applies to the parent
class(es?) (i.e. module-level to class-level).
- You should make `list` into a `cpdef` with an output type of `list` and
remove `_list_c`.
- For `__iter__`, just iterate over `self.__coeffs`. Actually, a similar
statement applies to many other places you call `_list_c` where you don't
expose it to the user or modify the list (e.g., `__getitem__` and
`_richcmp_`. The multivariate and sparse versions should be overriding
these methods anyways.
- You should use the cached version of `0` instead of going through
coercion and getting a new object in memory:
{{{#!diff
try:
l = (<SkewPolynomial>self)._list_c()[n]
return l
except IndexError:
- c = self.base_ring()(0)
- return c
+ return self.base_ring().zero()
}}}
- Use Python3 syntax for errors: `raise IndexError("skew polynomials are
immutable")`.
- Similar to above: `self.parent()(0)` -> `self.parent().zero()`.
- `is_nilpotent` should have a short description of what the method should
do (as well as the intended operation).
- `Returns` -> `Return` in a few first-line of docstrings.
- `.. Note::` -> `.. NOTE::` (this is a consistency thing).
- You have a blank `.. SEEALSO::` in `__floordiv__`.
- `is_left_divisible_by` doesn't have a short description. Similar for
other such methods.
- Error messages should start with a lowercase letter (this was something
we've agreed upon in somewhat more recent history).
- There is a preference to have {{{``True``}}} in code formatting since it
is a Python object/concept.
- Actually, a number of functions that use `_list_c` should be moved to
the concrete class (and maybe an `@abstract_method` placeholder done in
the ABC.
I might have some more comments later too. However this is looking very
good and it will be a great addition to Sage.
Also, the `_richcmp_` looks correct (although I don't have time right now
to run any serious tests).
--
Ticket URL: <https://trac.sagemath.org/ticket/13215#comment:92>
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 unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at https://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
