Sage: Ticket #13215: Skew polynomials
https://trac.sagemath.org/ticket/13215
<p>
If R is a ring equipped with an endomorphism sigma, the ring of skew polynomials over (R,sigma) is the ring of usual polynomials over R with the modified multiplication given by the rule X*a = sigma(a)*X.
</p>
<p>
Skew polynomials play an important role in several domains like coding theory or Galois representations theory in positive characteristic.
</p>
<p>
The attached patch provides:
</p>
<ol><li>a basic implementation of skew polynomials over any commutative ring (including addition, multiplication, euclidean division, gcd...)
</li><li>a more complete implementation of skew polynomials over finite fields (including factoring)
</li></ol><p>
NB: This ticket depends on tickets <a class="closed ticket" href="https://trac.sagemath.org/ticket/13214" title="enhancement: Finite field homomorphisms and Frobenius endomorphisms (closed: fixed)">#13214</a> (which implements Frobenius endomorphisms over finite fields) and <a class="closed ticket" href="https://trac.sagemath.org/ticket/13303" title="defect: is_unit and __invert__ for Polynomial Quotient Rings (closed: fixed)">#13303</a> (which fixes a bug in quotient_polynomial_ring_element.pyx). For convenience, I reattach the corresponding patches here (you need to apply these two patches first).
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/13215
Trac 1.1.6carusoMon, 09 Jul 2012 09:09:48 GMTstatus changed
https://trac.sagemath.org/ticket/13215#comment:1
https://trac.sagemath.org/ticket/13215#comment:1
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
</ul>
TicketcarusoMon, 09 Jul 2012 09:22:02 GMTdescription changed; dependencies set
https://trac.sagemath.org/ticket/13215#comment:2
https://trac.sagemath.org/ticket/13215#comment:2
<ul>
<li><strong>dependencies</strong>
set to <em>#13214</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/13215?action=diff&version=2">diff</a>)
</li>
</ul>
TicketcarusoMon, 09 Jul 2012 22:47:03 GMTattachment set
https://trac.sagemath.org/ticket/13215
https://trac.sagemath.org/ticket/13215
<ul>
<li><strong>attachment</strong>
set to <em>trac_13215_skew_polynomials.2.patch</em>
</li>
</ul>
TicketcarusoMon, 09 Jul 2012 22:48:26 GMT
https://trac.sagemath.org/ticket/13215#comment:3
https://trac.sagemath.org/ticket/13215#comment:3
<p>
File trac_13215_skew_polynomials.2.patch replaces trac_13215_skew_polynomials.patch
</p>
TickettfeulnerThu, 26 Jul 2012 05:55:21 GMTstatus changed; cc set
https://trac.sagemath.org/ticket/13215#comment:4
https://trac.sagemath.org/ticket/13215#comment:4
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>cc</strong>
<em>tfeulner</em> added
</li>
</ul>
<p>
With your changes it is now possible to test some element in an Univariate Quotient Polynomial Ring to be a unit and to compute its inverse. Unfortunately, this computation gives wrong results:
</p>
<pre class="wiki">sage: Z16x.<x> = Integers(16)[]
sage: GR.<y> = Z16x.quotient(x**2 + x+1 )
sage: (2*y).is_unit()
True
sage: (2*y)^(-1)
15*y + 15
sage: (2*y)*(2*y)^(-1)
2
</pre><p>
Thomas
</p>
TicketcarusoThu, 26 Jul 2012 21:17:41 GMT
https://trac.sagemath.org/ticket/13215#comment:5
https://trac.sagemath.org/ticket/13215#comment:5
<p>
Thanks for catching this!
</p>
<p>
I've updated my patch. (I've simply decided to raise a <a class="missing wiki">NotImplementedError?</a> when the base ring is not a field and, by the way, I've added some doctests.)
</p>
<p>
PS: apply patch trac_13215_skew_polynomials.patch and forget trac_13215_skew_polynomials.2.patch (is it possible to remove it?)
</p>
TicketcarusoThu, 26 Jul 2012 21:18:05 GMTstatus changed
https://trac.sagemath.org/ticket/13215#comment:6
https://trac.sagemath.org/ticket/13215#comment:6
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
TickettfeulnerFri, 27 Jul 2012 06:40:10 GMT
https://trac.sagemath.org/ticket/13215#comment:7
https://trac.sagemath.org/ticket/13215#comment:7
<p>
Maybe you should produce a seperate trac ticket for the fix of
</p>
<pre class="wiki">sage: (2*y)^(-1)
...
NotImplementedError: The base ring (=Ring of integers modulo 16) is not a field
</pre><p>
By the way, I know that this is a problem which also occurs for usual polynomials (because of the default definition of the reduce function). Do you have any idea how to fix this:
</p>
<pre class="wiki">sage: Z16x.<x> = SkewPolynomialRing(Integers(16))
sage: # Z16x.<x> = Integers(16)[] # has the same behaviour
sage: GR.<y> = Z16x.quotient(x**2 + x+1 )
sage: I = GR.ideal(4)
sage: I.reduce(6)
6
</pre>
TicketcarusoFri, 27 Jul 2012 08:28:29 GMTattachment set
https://trac.sagemath.org/ticket/13215
https://trac.sagemath.org/ticket/13215
<ul>
<li><strong>attachment</strong>
set to <em>trac_13303_invert_polynomial_quotient_rings.patch</em>
</li>
</ul>
TicketcarusoFri, 27 Jul 2012 08:30:23 GMT
https://trac.sagemath.org/ticket/13215#comment:8
https://trac.sagemath.org/ticket/13215#comment:8
<p>
Ok, I split my patch in two parts and I open a new ticket (cf <a class="closed ticket" href="https://trac.sagemath.org/ticket/13303" title="defect: is_unit and __invert__ for Polynomial Quotient Rings (closed: fixed)">#13303</a>).
</p>
<p>
I'm afraid I don't understand quite well the problem with reduce. Could you explain it a bit more please?
</p>
TicketcarusoFri, 27 Jul 2012 08:32:08 GMTdependencies, description changed
https://trac.sagemath.org/ticket/13215#comment:9
https://trac.sagemath.org/ticket/13215#comment:9
<ul>
<li><strong>dependencies</strong>
changed from <em>#13214</em> to <em>#13214, #13303</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/13215?action=diff&version=9">diff</a>)
</li>
</ul>
TicketcarusoFri, 27 Jul 2012 08:43:02 GMTcc changed
https://trac.sagemath.org/ticket/13215#comment:10
https://trac.sagemath.org/ticket/13215#comment:10
<ul>
<li><strong>cc</strong>
<em>caruso</em> added
</li>
</ul>
TicketjdemeyerFri, 27 Jul 2012 20:39:14 GMT
https://trac.sagemath.org/ticket/13215#comment:11
https://trac.sagemath.org/ticket/13215#comment:11
<p>
Please fill in your real name as Author.
</p>
TicketcarusoSat, 28 Jul 2012 07:40:10 GMTauthor set
https://trac.sagemath.org/ticket/13215#comment:12
https://trac.sagemath.org/ticket/13215#comment:12
<ul>
<li><strong>author</strong>
set to <em>Xavier Caruso</em>
</li>
</ul>
TickettfeulnerWed, 01 Aug 2012 05:59:19 GMT
https://trac.sagemath.org/ticket/13215#comment:13
https://trac.sagemath.org/ticket/13215#comment:13
<p>
Hi Xavier,
</p>
<p>
you are right this is a more general problem. I started a discussion on
<a class="ext-link" href="https://groups.google.com/d/topic/sage-devel/s5y604ZPiQ8/discussion"><span class="icon"></span>https://groups.google.com/d/topic/sage-devel/s5y604ZPiQ8/discussion</a>.
The function reduce() does what it says <em>returning an element of the ring</em>.
But in the definition of a <a class="missing wiki">QuotientRing?</a> there is the following assumption
</p>
<blockquote>
<p>
<code>I.reduce(x)==I.reduce(y) \iff x-y \in I</code>, and
<code>x-I.reduce(x) \in I</code>, for all <code>x \in R</code>.
</p>
</blockquote>
<p>
With this default behaviour, the quotient ring is just a copy of the original ring R.
</p>
<p>
Best, Thomas
</p>
TicketburcinSun, 26 Aug 2012 22:19:53 GMTcc, component changed
https://trac.sagemath.org/ticket/13215#comment:14
https://trac.sagemath.org/ticket/13215#comment:14
<ul>
<li><strong>cc</strong>
<em>burcin</em> added
</li>
<li><strong>component</strong>
changed from <em>experimental package</em> to <em>algebra</em>
</li>
</ul>
<p>
It would be great to have all this in Sage. Thanks a lot for your work.
</p>
<p>
I haven't had a chance to look at the patches yet. I will try to make some time available for this in the coming weeks. For now, I have two questions,
</p>
<ul><li>Did you consider using Singular's noncommutative component, Plural, as a basic data structure for skew polynomials? I expect the arithmetic to be much faster with Plural than a new implementation in Cython.
</li><li>Can the patches be broken down into smaller components for easier review?
</li></ul>
TicketcarusoTue, 28 Aug 2012 19:34:31 GMT
https://trac.sagemath.org/ticket/13215#comment:15
https://trac.sagemath.org/ticket/13215#comment:15
<p>
I haven't heard about Plural before that... so, I haven't considered using it for skew polynomials yet. I will compare how fast is the arithmetic with Plural and with my own implementation in Cython and let you know.
</p>
<p>
Ok, I will split my patch in several components. (Is two enough or are you expecting more than that?).
</p>
TicketburcinWed, 29 Aug 2012 10:16:05 GMT
https://trac.sagemath.org/ticket/13215#comment:16
https://trac.sagemath.org/ticket/13215#comment:16
<p>
Thanks for the prompt response! I am attending a workshop this week and I haven't had a chance to read through your patch properly yet. So please excuse me if I am completely off the mark in my response below. :)
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:15" title="Comment 15">caruso</a>:
</p>
<blockquote class="citation">
<p>
I haven't heard about Plural before that... so, I haven't considered using it for skew polynomials yet. I will compare how fast is the arithmetic with Plural and with my own implementation in Cython and let you know.
</p>
</blockquote>
<p>
Your patch provides more functionality than Plural, for example gcd's and factoring. The coefficient domains supported by Plural are also limited. I suppose your implementation supports more general coefficient domains, basically anything Sage already knows about.
</p>
<p>
It would be good to have a clear comparison between Plural and your implementation first. But I imagine having two skew polynomial classes in Sage, a generic implementation from your patch and one based on Plural. We could probably share the high level functionality not provided by Plural using the category framework.
</p>
<p>
I am not opposed to just including your patch and think about the Plural part later as an optimization if you say that your implementation is more capable.
</p>
<blockquote class="citation">
<p>
Ok, I will split my patch in several components. (Is two enough or are you expecting more than that?).
</p>
</blockquote>
<p>
I don't know yet. It depends on how many functionally independent parts there are. For instance, the short representations for morphisms can be an independent piece that is trivial to review. The hash functions for morphisms would also be a separate bug fix. Actually, I attempted to fix that a long time ago at <a class="closed ticket" href="https://trac.sagemath.org/ticket/9016" title="defect: make morphisms hashable (closed: fixed)">#9016</a>.
</p>
TicketcarusoWed, 29 Aug 2012 12:23:06 GMT
https://trac.sagemath.org/ticket/13215#comment:17
https://trac.sagemath.org/ticket/13215#comment:17
<p>
I had a quick look at the documentation of Plural but I didn't find how to create a skew polynomial ring (i.e. I don't know how to represent a skew polynomial ring as a G-algebra). Could you please learn me this?
</p>
TicketburcinMon, 08 Oct 2012 13:21:03 GMTdescription changed
https://trac.sagemath.org/ticket/13215#comment:18
https://trac.sagemath.org/ticket/13215#comment:18
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/13215?action=diff&version=18">diff</a>)
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:17" title="Comment 17">caruso</a>:
</p>
<blockquote class="citation">
<p>
I had a quick look at the documentation of Plural but I didn't find how to create a skew polynomial ring (i.e. I don't know how to represent a skew polynomial ring as a G-algebra). Could you please learn me this?
</p>
</blockquote>
<p>
For example, QQ[n, S:n->n+1] :
</p>
<pre class="wiki">sage: A.<n, S> = FreeAlgebra(QQ, 2)
sage: R.<n, S> = A.g_algebra({S*n: n*S + 1})
sage: R
Noncommutative Multivariate Polynomial Ring in n, S over Rational Field, nc-relations: {S*n: n*S + 1}
sage: S*n
n*S + 1
sage: S^2*n
n*S^2 + 2*S
</pre><p>
After applying the patches on this ticket and its dependencies, I get the following error:
</p>
<pre class="wiki">sage: R.<t> = ZZ[]
sage: sigma = R.hom([t+1])
sage: S.<x> = R['x',sigma]; S
Skew Polynomial Ring in x over Univariate Polynomial Ring in t over Integer Ring twisted by t |--> t + 1
sage: x*t
---------------------------------------------------------------------------
NotImplementedError Traceback (most recent call last)
...
sage: %debug
> /home/burcin/sage/sage-5.2/skewpolynomial_element.pyx(347)sage.rings.polynomial.skewpolynomial_element.SkewPolynomial._list_c (sage/rings/polynomial/skewpolynomial_element.c:4716)()
</pre>
TicketburcinMon, 08 Oct 2012 14:10:39 GMTstatus changed; reviewer set
https://trac.sagemath.org/ticket/13215#comment:19
https://trac.sagemath.org/ticket/13215#comment:19
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>reviewer</strong>
set to <em>Burcin Erocal</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:16" title="Comment 16">burcin</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:15" title="Comment 15">caruso</a>:
</p>
</blockquote>
<blockquote class="citation">
<blockquote class="citation">
<p>
Ok, I will split my patch in several components. (Is two enough or are you expecting more than that?).
</p>
</blockquote>
<p>
I don't know yet. It depends on how many functionally independent parts there are. For instance, the short representations for morphisms can be an independent piece that is trivial to review. The hash functions for morphisms would also be a separate bug fix. Actually, I attempted to fix that a long time ago at <a class="closed ticket" href="https://trac.sagemath.org/ticket/9016" title="defect: make morphisms hashable (closed: fixed)">#9016</a>.
</p>
</blockquote>
<p>
From a cursory reading of <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/13215/trac_13215_skew_polynomials.patch" title="Attachment 'trac_13215_skew_polynomials.patch' in Ticket #13215">attachment:trac_13215_skew_polynomials.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/13215/trac_13215_skew_polynomials.patch" title="Download"></a>, I see these separate parts that should have a ticket of its own:
</p>
<ul><li>short representations for morphisms
</li><li>hash functions for morphisms (<a class="closed ticket" href="https://trac.sagemath.org/ticket/9016" title="defect: make morphisms hashable (closed: fixed)">#9016</a>)
</li><li>q_jordan
</li><li>modular exponentiation of polynomial_element
</li></ul><p>
A few comments (not a full review):
</p>
<ul><li>the operator <code>/</code> always constructs the quotient domain in Sage, even if exact division is possible. Since in this case we need a generic Ore localization for <code>/</code>, I suggest you only implement exact division with <code>//</code> and raise an error on <code>/</code>.
<pre class="wiki">+ sage: d = a * b
+ sage: d/b == a # Right division
+ True
+
+ sage: c/b
+ Traceback (most recent call last):
+ ...
+ ValueError: the division is not exact
</pre></li></ul><ul><li>For the constructor, why don't you check if the argument of the <code>__getitem__</code> method is a <code>Morphism</code> instead of this:
<pre class="wiki"> from sage.categories.morphism import Morphism
if isinstance(x,tuple) and len(x) == 2 and isinstance(x[1],Morphism):
</pre>This should remove the limitation of having to specify the variable name on the right hand side as documented here:
<pre class="wiki">+One can also use a shorter syntax::
+
+ sage: S.<x> = R['x',sigma]; S
+ Skew Polynomial Ring in x over Univariate Polynomial Ring in t over Integer Ring twisted by t |--> t + 1
+
+Be careful, with the latter syntax, one cannot omit the name of the
+variable neither in LHS nor in RHS. If we omit it in LHS, the variable
+is not created::
+
+ sage: Sy = R['y',sigma]; Sy
+ Skew Polynomial Ring in y over Univariate Polynomial Ring in t over Integer Ring twisted by t |--> t + 1
+ sage: y.parent()
+ Traceback (most recent call last):
+ ...
+ NameError: name 'y' is not defined
+
+If we omit it in RHS, sage tries to create a polynomial ring and fails::
+
+ sage: Sz.<z> = R[sigma]
+ Traceback (most recent call last):
+ ...
+ ValueError: variable names must be alphanumeric, but one is 'Ring endomorphism of Univariate Polynomial Ring in t over Integer Ring
+ Defn: t |--> t + 1' which is not.
</pre></li></ul><ul><li>There is a class <code>SkewPolynomialRing_finite_field</code> defined in <code>sage/rings/polynomial/skewpolynomial_ring.py</code>. There is also a file <code>sage/rings/polynomial/skewpolynomial_finite_field.pyx</code>. It seems that the class defined in the latter does not use the latter:
<pre class="wiki">cdef class SkewPolynomial_finite_field_dense (SkewPolynomial_generic_dense):
</pre></li></ul><ul><li>I suggest using <code>skew_polynomial</code> in the file names instead of <code>skewpolynomial</code>.
</li></ul><ul><li>Are the <code>Left</code> and <code>Right</code> objects defined in <code>sage/structure/side.py</code> really necessary? If <code>Right</code> is the default, can't you just have a keyword argument <code>left=<bool></code>?
</li></ul><ul><li>There are many functions missing a docstring or tests. Here is partial coverage output:
<pre class="wiki">$ ./sage -coverage devel/sage/sage/rings/polynomial/skew*
----------------------------------------------------------------------
devel/sage/sage/rings/polynomial/skewpolynomial_element.pyx
ERROR: Please add a `TestSuite(s).run()` doctest.
SCORE devel/sage/sage/rings/polynomial/skewpolynomial_element.pyx: 78% (58 of 74)
Missing documentation:
* __hash__(self):
* __richcmp__(left, right, int op):
* __nonzero__(self):
* _is_atomic(self):
* __lshift__(self, k):
* __rshift__(self, k):
* is_gen(self):
* make_generic_skewpolynomial(parent, coeffs):
* Element _call_(self, x):
* __init__(self, domain, codomain):
* Element _call_(self, x):
* Element _call_with_args(self, x, args=(), kwds={}):
* section(self):
Missing doctests:
* SkewPolynomial _new_constant_poly(self,RingElement a,Parent P,char check=0):
* is_unit(self):
* __copy__(self):
Possibly wrong (function name doesn't occur in doctests):
* ModuleElement _add_(self, ModuleElement right):
* ModuleElement _sub_(self, ModuleElement right):
* ModuleElement _neg_(self):
* ModuleElement _lmul_(self, RingElement right):
* ModuleElement _rmul_(self, RingElement left):
* RingElement _mul_(self, RingElement right):
* RingElement _div_(self, RingElement right):
----------------------------------------------------------------------
----------------------------------------------------------------------
devel/sage/sage/rings/polynomial/skewpolynomial_finite_field.pyx
ERROR: Please add a `TestSuite(s).run()` doctest.
SCORE devel/sage/sage/rings/polynomial/skewpolynomial_finite_field.pyx: 82% (29 of 35)
Missing documentation:
* __init__(self, parent, x=None, int check=1, is_gen=False, int construct=0, **kwds):
* reduced_norm_factor(self):
* _factorizations_rec(self):
Missing doctests:
* _irreducible_divisors(self,side):
* __init__(self,parent,cutoff=0):
* __repr__(self):
Possibly wrong (function name doesn't occur in doctests):
* RingElement _mul_(self, RingElement right):
----------------------------------------------------------------------
----------------------------------------------------------------------
devel/sage/sage/rings/polynomial/skewpolynomial_ring_constructor.py
SCORE devel/sage/sage/rings/polynomial/skewpolynomial_ring_constructor.py: 100% (1 of 1)
----------------------------------------------------------------------
----------------------------------------------------------------------
devel/sage/sage/rings/polynomial/skewpolynomial_ring.py
SCORE devel/sage/sage/rings/polynomial/skewpolynomial_ring.py: 42% (17 of 40)
Missing documentation:
* _call_ (self, x):
* __init__(self,domain,codomain,embed,order):
* _repr_(self):
* _call_(self,x):
* section(self):
* __init__ (self, skew_ring, names=None, sparse=False, element_class=None):
* __classcall__(cls, base_ring, map, name=None, sparse=False, element_class=None):
* __init__(self, base_ring, map, name, sparse, element_class):
* __reduce__(self):
* _element_constructor_(self, x=None, check=True, is_gen = False, construct=False, **kwds):
* _coerce_map_from_(self, P):
* __cmp__(left, right):
* _repr_(self):
* _latex_(self):
* gens_dict(self):
* __classcall__(cls, base_ring, map, name=None, sparse=False, element_class=None):
* __init__(self, base_ring, map, name, sparse, element_class):
Missing doctests:
* _repr_ (self):
* _latex_ (self):
* parameter(self):
* is_sparse(self):
* _new_retraction_map(self,alea=None):
* _retraction(self,x,newmap=False,alea=None):
----------------------------------------------------------------------
</pre></li></ul>
TicketburcinWed, 10 Oct 2012 13:51:46 GMT
https://trac.sagemath.org/ticket/13215#comment:20
https://trac.sagemath.org/ticket/13215#comment:20
<p>
Here is some more information from Viktor Levandovskyy about Plural and skew polynomials rings over finite fields with the frobenius map.
</p>
<pre class="wiki">> Plural can be used in the following situations:
> Let |F|=p^k, where p is a prime.
> *) always possible: k = 1,2 and k = p-1,p.
> *) For k=3,...,p-2 there's no general statement, sometimes
> works, sometimes not. Depends on p and k
> *) k>p: impossible (with Plural)
</pre>
TicketcarusoMon, 22 Oct 2012 14:42:22 GMTstatus, dependencies, description changed
https://trac.sagemath.org/ticket/13215#comment:21
https://trac.sagemath.org/ticket/13215#comment:21
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>dependencies</strong>
changed from <em>#13214, #13303</em> to <em>#13214, #13303, #13640, #13641, #13642</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/13215?action=diff&version=21">diff</a>)
</li>
</ul>
<p>
Thanks a lot for all your comments!
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:19" title="Comment 19">burcin</a>:
</p>
<blockquote class="citation">
<ul><li>short representations for morphisms
</li><li>hash functions for morphisms (<a class="closed ticket" href="https://trac.sagemath.org/ticket/9016" title="defect: make morphisms hashable (closed: fixed)">#9016</a>)
</li><li>q_jordan
</li><li>modular exponentiation of polynomial_element
</li></ul></blockquote>
<p>
Ok, I've done it (see tickets <a class="closed ticket" href="https://trac.sagemath.org/ticket/13640" title="enhancement: q-numbers coutings flags stable under a nilpotent endomorphism (closed: fixed)">#13640</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/13641" title="enhancement: Short representation of morphisms (closed: fixed)">#13641</a> <a class="closed ticket" href="https://trac.sagemath.org/ticket/13642" title="defect: Modular exponentiation of polynomials (closed: fixed)">#13642</a> and old ticket <a class="closed ticket" href="https://trac.sagemath.org/ticket/13214" title="enhancement: Finite field homomorphisms and Frobenius endomorphisms (closed: fixed)">#13214</a> for "hash functions for morphisms").
</p>
<blockquote class="citation">
<p>
After applying the patches on this ticket and its dependencies, I get the following error:
</p>
<pre class="wiki">sage: R.<t> = ZZ[]
sage: sigma = R.hom([t+1])
sage: S.<x> = R['x',sigma]; S
Skew Polynomial Ring in x over Univariate Polynomial Ring in t over Integer Ring twisted by t |--> t + 1
sage: x*t
---------------------------------------------------------------------------
NotImplementedError Traceback (most recent call last)
...
sage: %debug
> /home/burcin/sage/sage-5.2/skewpolynomial_element.pyx(347)sage.rings.polynomial.skewpolynomial_element.SkewPolynomial._list_c (sage/rings/polynomial/skewpolynomial_element.c:4716)()
</pre></blockquote>
<p>
It's now fixed. (I haven't really understood what was the problem. It worked well on sage 5.1.0. I just removed some <code>inline</code> somewhere.)
</p>
<blockquote class="citation">
<ul><li>the operator <code>/</code> always constructs the quotient domain in Sage, even if exact division is possible. Since in this case we need a generic Ore localization for <code>/</code>, I suggest you only implement exact division with <code>//</code> and raise an error on <code>/</code>.
</li></ul></blockquote>
<p>
Done.
</p>
<blockquote class="citation">
<ul><li>For the constructor, why don't you check if the argument of the <code>__getitem__</code> method is a <code>Morphism</code> instead of this:
<pre class="wiki"> from sage.categories.morphism import Morphism
if isinstance(x,tuple) and len(x) == 2 and isinstance(x[1],Morphism):
</pre>This should remove the limitation of having to specify the variable name on the right hand side as documented here:
</li></ul></blockquote>
<p>
It's not possible to remove this limitation this way because of the preparser (you see in the following example that the variable 'x' doesn't appear in the first instruction on the last line).
</p>
<pre class="wiki">sage: k.<t> = GF(5^3)
sage: Frob = k.frobenius_endomorphism()
sage: preparse(S.<x> = k[Frob])
sage: preparse("S.<x> = k[Frob]")
'S = k[Frob]; (x,) = S._first_ngens(1)'
</pre><blockquote class="citation">
<ul><li>I suggest using <code>skew_polynomial</code> in the file names instead of <code>skewpolynomial</code>.
</li></ul></blockquote>
<p>
Done.
</p>
<blockquote class="citation">
<ul><li>Are the <code>Left</code> and <code>Right</code> objects defined in <code>sage/structure/side.py</code> really necessary? If <code>Right</code> is the default, can't you just have a keyword argument <code>left=<bool></code>?
</li></ul></blockquote>
<p>
Sometimes, <code>Left</code> is the default (for lcm). I agree that we can just have a boolean argument but, when I wrote this code, I thought that it was more elegant to do this way. Anymay, I can change if you insist :-).
</p>
<blockquote class="citation">
<ul><li>There are many functions missing a docstring or tests.
</li></ul></blockquote>
<p>
I've added some documentation and doctests (but I'm still far from a 100% coverage).
</p>
TicketcarusoMon, 22 Oct 2012 14:44:19 GMTattachment set
https://trac.sagemath.org/ticket/13215
https://trac.sagemath.org/ticket/13215
<ul>
<li><strong>attachment</strong>
set to <em>trac_13640_qjordan.patch</em>
</li>
</ul>
TicketcarusoMon, 22 Oct 2012 14:44:27 GMTattachment set
https://trac.sagemath.org/ticket/13215
https://trac.sagemath.org/ticket/13215
<ul>
<li><strong>attachment</strong>
set to <em>trac_13641_short_repr_morphism.patch</em>
</li>
</ul>
TicketdarijMon, 13 May 2013 05:07:47 GMTcc changed
https://trac.sagemath.org/ticket/13215#comment:22
https://trac.sagemath.org/ticket/13215#comment:22
<ul>
<li><strong>cc</strong>
<em>darij</em> added
</li>
</ul>
TicketcarusoWed, 26 Jun 2013 10:27:03 GMTattachment set
https://trac.sagemath.org/ticket/13215
https://trac.sagemath.org/ticket/13215
<ul>
<li><strong>attachment</strong>
set to <em>trac_13642_modular_exp_polynomial.patch</em>
</li>
</ul>
TicketcarusoWed, 26 Jun 2013 10:28:10 GMTattachment set
https://trac.sagemath.org/ticket/13215
https://trac.sagemath.org/ticket/13215
<ul>
<li><strong>attachment</strong>
set to <em>trac_13214_hom_finite_field.patch</em>
</li>
</ul>
TicketcarusoWed, 26 Jun 2013 10:30:54 GMTdescription changed
https://trac.sagemath.org/ticket/13215#comment:23
https://trac.sagemath.org/ticket/13215#comment:23
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/13215?action=diff&version=23">diff</a>)
</li>
</ul>
<p>
I've posted an updated version of this patch which should apply at the top of sage 5-10 (hopefully).
</p>
TicketcarusoWed, 26 Jun 2013 10:32:56 GMTattachment set
https://trac.sagemath.org/ticket/13215
https://trac.sagemath.org/ticket/13215
<ul>
<li><strong>attachment</strong>
set to <em>trac_13215_skew_polynomials.patch</em>
</li>
</ul>
TicketjdemeyerTue, 13 Aug 2013 15:35:53 GMTmilestone changed
https://trac.sagemath.org/ticket/13215#comment:24
https://trac.sagemath.org/ticket/13215#comment:24
<ul>
<li><strong>milestone</strong>
changed from <em>sage-5.11</em> to <em>sage-5.12</em>
</li>
</ul>
TicketdarijThu, 12 Sep 2013 13:10:08 GMT
https://trac.sagemath.org/ticket/13215#comment:25
https://trac.sagemath.org/ticket/13215#comment:25
<p>
Needs to be rebased on sage-5.12beta5. The patch fails to apply to sage/structure/all.py.
</p>
<p>
I'd do it myself but when I try to open the file in gedit, I get:
"The file you opened has some invalid characters. If you continue editing this file you could corrupt this document.
You can also choose another character encoding and try again."
Might be some accents or umlauts on names.
</p>
Ticketvbraun_spamThu, 30 Jan 2014 21:20:52 GMTmilestone changed
https://trac.sagemath.org/ticket/13215#comment:26
https://trac.sagemath.org/ticket/13215#comment:26
<ul>
<li><strong>milestone</strong>
changed from <em>sage-6.1</em> to <em>sage-6.2</em>
</li>
</ul>
Ticketvbraun_spamTue, 06 May 2014 15:20:58 GMTmilestone changed
https://trac.sagemath.org/ticket/13215#comment:27
https://trac.sagemath.org/ticket/13215#comment:27
<ul>
<li><strong>milestone</strong>
changed from <em>sage-6.2</em> to <em>sage-6.3</em>
</li>
</ul>
Ticketvbraun_spamSun, 10 Aug 2014 16:51:03 GMTmilestone changed
https://trac.sagemath.org/ticket/13215#comment:28
https://trac.sagemath.org/ticket/13215#comment:28
<ul>
<li><strong>milestone</strong>
changed from <em>sage-6.3</em> to <em>sage-6.4</em>
</li>
</ul>
TicketmmezzarobbaSun, 08 Feb 2015 10:23:11 GMTstatus changed; work_issues set
https://trac.sagemath.org/ticket/13215#comment:29
https://trac.sagemath.org/ticket/13215#comment:29
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>work_issues</strong>
set to <em>see comment #25</em>
</li>
</ul>
Ticketpanda314Sat, 12 Mar 2016 07:57:41 GMTbranch set
https://trac.sagemath.org/ticket/13215#comment:30
https://trac.sagemath.org/ticket/13215#comment:30
<ul>
<li><strong>branch</strong>
set to <em>u/panda314/skew_polynomials</em>
</li>
</ul>
TicketgitSat, 12 Mar 2016 08:16:41 GMTcommit set
https://trac.sagemath.org/ticket/13215#comment:31
https://trac.sagemath.org/ticket/13215#comment:31
<ul>
<li><strong>commit</strong>
set to <em>f6305cee0d39c2a1a3727482af453c59791a9e1c</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=f6305cee0d39c2a1a3727482af453c59791a9e1c"><span class="icon"></span>f6305ce</a></td><td><code>Revert "adding examples involving min_symbolic and max_symbolic"(did changes in the wrong ticket)</code>
</td></tr></table>
TicketarpitdmMon, 13 Jun 2016 15:21:53 GMTbranch changed
https://trac.sagemath.org/ticket/13215#comment:32
https://trac.sagemath.org/ticket/13215#comment:32
<ul>
<li><strong>branch</strong>
changed from <em>u/panda314/skew_polynomials</em> to <em>u/arpitdm/skew_polynomials</em>
</li>
</ul>
TicketarpitdmMon, 13 Jun 2016 15:26:33 GMTcc, commit changed
https://trac.sagemath.org/ticket/13215#comment:33
https://trac.sagemath.org/ticket/13215#comment:33
<ul>
<li><strong>cc</strong>
<em>jsrn</em> <em>dlucas</em> added
</li>
<li><strong>commit</strong>
changed from <em>f6305cee0d39c2a1a3727482af453c59791a9e1c</em> to <em>de61ca43086bcdef18a4171915064fdc469d2036</em>
</li>
</ul>
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=d915c13f23ca703701985f8df97f0c196332ccbe"><span class="icon"></span>d915c13</a></td><td><code>merging the skew_polynomial ticket 13215 in local repository</code>
</td></tr><tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=447ddb3e1b917c936a61109bae73e0993b1c87b9"><span class="icon"></span>447ddb3</a></td><td><code>Extensions for skew_polynomial_element and skew_polynomial_finite field added.</code>
</td></tr><tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=05cc01bb67afa4139e0eb113d78bb46428e3fbe9"><span class="icon"></span>05cc01b</a></td><td><code>The methods PY_NEW_SAME_TYPE and PY_TYPE_CHECK_EXACT are not supported by Sage anymore and are replaced by the method type. The Cython comparison function is updated to _cmp_ supported in other rings in Sage.</code>
</td></tr><tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=241b98d4a2642949e534a561c7a885705bafe350"><span class="icon"></span>241b98d</a></td><td><code>The methods PY_NEW_SAME_TYPE and PY_TYPE_CHECK_EXACT are not supported by Sage anymore and are replaced by the method type.</code>
</td></tr><tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=98277cd9bc03a661abf74bbf4f639c686caa2886"><span class="icon"></span>98277cd</a></td><td><code>Rewrote the construction of skew polynomial ring according to the new conventions. Original code had placed it in the /sage/rings/rings.pyx file but the current location has moved to the getitem method of /sage/category/ring.py</code>
</td></tr><tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=de61ca43086bcdef18a4171915064fdc469d2036"><span class="icon"></span>de61ca4</a></td><td><code>Importing ring_element, integral_domain and principal_ideal_domain were throwing deprecation warnings. Updated the import statements according to Tickets 19167 and 20011.</code>
</td></tr></table>
TicketgitThu, 16 Jun 2016 10:05:28 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:34
https://trac.sagemath.org/ticket/13215#comment:34
<ul>
<li><strong>commit</strong>
changed from <em>de61ca43086bcdef18a4171915064fdc469d2036</em> to <em>9d21b5420c9464630854756ad4747ab766b96f4e</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=9d21b5420c9464630854756ad4747ab766b96f4e"><span class="icon"></span>9d21b54</a></td><td><code>module that implements class 'Side' with Left and Right as the two instances.</code>
</td></tr></table>
TickettscrimThu, 16 Jun 2016 18:13:08 GMTcc changed
https://trac.sagemath.org/ticket/13215#comment:35
https://trac.sagemath.org/ticket/13215#comment:35
<ul>
<li><strong>cc</strong>
<em>tscrim</em> added
</li>
</ul>
TicketgitTue, 21 Jun 2016 06:33:13 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:36
https://trac.sagemath.org/ticket/13215#comment:36
<ul>
<li><strong>commit</strong>
changed from <em>9d21b5420c9464630854756ad4747ab766b96f4e</em> to <em>9c526ad250d8053e5b9184219e7e3cfc4186c097</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=9c526ad250d8053e5b9184219e7e3cfc4186c097"><span class="icon"></span>9c526ad</a></td><td><code>Fixed doctests caused by deprecation warnings, inability to access private variables, AttributeError in accessing of parent, and .</code>
</td></tr></table>
TicketdlucasFri, 24 Jun 2016 14:53:44 GMTbranch changed
https://trac.sagemath.org/ticket/13215#comment:37
https://trac.sagemath.org/ticket/13215#comment:37
<ul>
<li><strong>branch</strong>
changed from <em>u/arpitdm/skew_polynomials</em> to <em>u/dlucas/skew_polynomials</em>
</li>
</ul>
TicketdlucasFri, 24 Jun 2016 14:55:11 GMTcommit, milestone changed
https://trac.sagemath.org/ticket/13215#comment:38
https://trac.sagemath.org/ticket/13215#comment:38
<ul>
<li><strong>commit</strong>
changed from <em>9c526ad250d8053e5b9184219e7e3cfc4186c097</em> to <em>18c7982a3d363ad004760d91e874783eab4c72ae</em>
</li>
<li><strong>milestone</strong>
changed from <em>sage-6.4</em> to <em>sage-7.3</em>
</li>
</ul>
<p>
I fixed one of the remaining bugs related to coercion of integers to a skew polynomial ring element.
Some bugs still remain.
</p>
<hr />
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=9de7bc1a78c25351d479e65ba2c24dedec4e5200"><span class="icon"></span>9de7bc1</a></td><td><code>Merge branch 'u/arpitdm/skew_polynomials' of git://trac.sagemath.org/sage into skew_polynomials</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=a06c2bf94cdbc788510184451c69b52f0091bd3e"><span class="icon"></span>a06c2bf</a></td><td><code>Merge branch 'u/arpitdm/skew_polynomials' of git://trac.sagemath.org/sage into skew_polynomials</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=18c7982a3d363ad004760d91e874783eab4c72ae"><span class="icon"></span>18c7982</a></td><td><code>Fixed bug with integer coercion</code>
</td></tr></table>
TicketjsrnFri, 24 Jun 2016 15:08:04 GMT
https://trac.sagemath.org/ticket/13215#comment:39
https://trac.sagemath.org/ticket/13215#comment:39
<p>
A little thing I noted: <code>is_SkewPolynomial</code> should be removed, I think. Sage
moved away from that kind of functions. The category framework should be used
instead, but I don't think we need to find a replacement for it just now.
</p>
TickettscrimFri, 24 Jun 2016 15:46:57 GMT
https://trac.sagemath.org/ticket/13215#comment:40
https://trac.sagemath.org/ticket/13215#comment:40
<p>
A better idiom is to just use <code>isinstance(S, SkewPolynomialRing_general)</code> as it is explicit (and there is no category for skew polynomial rings).
</p>
TicketarpitdmThu, 07 Jul 2016 05:07:58 GMTbranch changed
https://trac.sagemath.org/ticket/13215#comment:41
https://trac.sagemath.org/ticket/13215#comment:41
<ul>
<li><strong>branch</strong>
changed from <em>u/dlucas/skew_polynomials</em> to <em>u/arpitdm/skew_polynomials</em>
</li>
</ul>
TicketjsrnSat, 09 Jul 2016 18:07:19 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:42
https://trac.sagemath.org/ticket/13215#comment:42
<ul>
<li><strong>commit</strong>
changed from <em>18c7982a3d363ad004760d91e874783eab4c72ae</em> to <em>dd5c575e26faaa88140e67c92130085b28e8b0a2</em>
</li>
</ul>
<p>
One of the bugs in the doctest has to do with failing in the proper manner when the twist map is not invertible (this is the <code>TypeError: bad operand type for unary ~</code> bug). This is because <code>self._map ** n</code> is being called with negative <code>n</code>, which will then try to invert <code>self._map</code> using the unary <code>~</code> operator which is not supported by <code>self._map</code> of type <code>Homomorphism_im_gens</code>.
</p>
<p>
But the code was always supposed to fail! The doc-test just expected the error <a class="missing wiki">NotImplementedError?</a>, while the error nowadays is this unhelpful unary-~-stuff.
</p>
<p>
One solution is therefore simply to catch the strange error and rethrow a <a class="missing wiki">NotImplementedException?</a>. The following patch should be put in <code>skew_polynomial_ring.py</code> (the line numbers are not completely correct):
</p>
<pre class="wiki">@@ -534,9 +534,18 @@ class SkewPolynomialRing_general(sage.algebras.algebra.Algebra,UniqueRepresentat
try:
return self._maps[n]
except KeyError:
- map = self._map**n
- self._maps[n] = map
- return map
+ if n >= 0:
+ map = self._map**n
+ self._maps[n] = map
+ return map
+ else:
+ try:
+ map = self._map**n
+ except TypeError:
+ raise NotImplementedError("Inversion of the twist map %s" % self._map)
+ self._maps[n] = map
+ return map
+
</pre><p>
Best,
Johan
</p>
<hr />
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=c6183bcfb9eb863f5052a3d3cd05e35e85a62c29"><span class="icon"></span>c6183bc</a></td><td><code>Merge branch 'develop' into temp3</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=dd5c575e26faaa88140e67c92130085b28e8b0a2"><span class="icon"></span>dd5c575</a></td><td><code>Editing declarations of cython functions so as to make them compatible.</code>
</td></tr></table>
TicketjsrnSat, 09 Jul 2016 18:09:10 GMT
https://trac.sagemath.org/ticket/13215#comment:43
https://trac.sagemath.org/ticket/13215#comment:43
<p>
Note that I didn't modify any code: the two commits just appear under my message because arpitdm didn't leave a comment after his last push.
</p>
TicketgitTue, 12 Jul 2016 09:22:38 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:44
https://trac.sagemath.org/ticket/13215#comment:44
<ul>
<li><strong>commit</strong>
changed from <em>dd5c575e26faaa88140e67c92130085b28e8b0a2</em> to <em>378b2cf7afb5d891b535b980fff85b4c9a6ef8ca</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=81a291f1dfcd1994be08dae258e3310790b51d75"><span class="icon"></span>81a291f</a></td><td><code>Added more sig_on/sig_off</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=dce652ee791dc7ad28789e405f37ce86bdd621d7"><span class="icon"></span>dce652e</a></td><td><code>Fix some imports</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=a7b8815571835e35d1fff2fc514ae4fe846b8ea1"><span class="icon"></span>a7b8815</a></td><td><code>commented out suspicious __copy__ method</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=dd821ec518f00d1af20d6188308e145a4b86fbbc"><span class="icon"></span>dd821ec</a></td><td><code>A few more sig_on/off and cleaning up some commented out code</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=d5fad11c38178622c2facb282a30b4cdf18070b7"><span class="icon"></span>d5fad11</a></td><td><code>sig_on/off almost everywhere in finite field pyx (omitted factoring/irreducible stuff)</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=df59050f07c0974df5bddbcea4fbed4086647726"><span class="icon"></span>df59050</a></td><td><code>Fixed bug with crashing add for finite fields by declaring SkewPolynomial.__normalize</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=a92d241e9b11f3e2b3eb1420d86c9e4327be9912"><span class="icon"></span>a92d241</a></td><td><code>Fixed segfault with _pow_ by declaring some _inplace_* already in SkewPolynomial class</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=bb36f9af7402402a3f4edffda150e5007baa5380"><span class="icon"></span>bb36f9a</a></td><td><code>Suspicious reuse of list of coefficients made into a safe copy</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=544ca8d7dfc6b3a4c5d9e469f4ffb031da7f4c48"><span class="icon"></span>544ca8d</a></td><td><code>Fix x._pow_(2) by calling ._new_c instead of _parent (I don't know why this worked!)</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=378b2cf7afb5d891b535b980fff85b4c9a6ef8ca"><span class="icon"></span>378b2cf</a></td><td><code>fixed indentation error</code>
</td></tr></table>
TicketarpitdmTue, 12 Jul 2016 09:25:10 GMT
https://trac.sagemath.org/ticket/13215#comment:45
https://trac.sagemath.org/ticket/13215#comment:45
<p>
Cherry-picked commits based on fixes by Johan. The segfault from before is now fixed. Various doctest errors still remain. Now breaking up this ticket into smaller pieces so that it will be easier to manage.
</p>
TicketgitTue, 12 Jul 2016 18:18:29 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:46
https://trac.sagemath.org/ticket/13215#comment:46
<ul>
<li><strong>commit</strong>
changed from <em>378b2cf7afb5d891b535b980fff85b4c9a6ef8ca</em> to <em>60dc3329dcaf4ce971b68867999f02d71b2f989a</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=f3a92090dcc01de87e731b0921294382ee1086be"><span class="icon"></span>f3a9209</a></td><td><code>removed skew_polynomial_finite_field files</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=09dc81d2228e806eb4c8a10a60c4d439fa41b321"><span class="icon"></span>09dc81d</a></td><td><code>removed the irreducibility, center and finite field stuff from this file</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=ff60603484a60202f150f57ed6e636860f95d48b"><span class="icon"></span>ff60603</a></td><td><code>removed finite_fields stuff from file</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=97c77701bfa895ce222e47acc4e8d9870b0d687b"><span class="icon"></span>97c7770</a></td><td><code>removed extension from module_list.py</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=60dc3329dcaf4ce971b68867999f02d71b2f989a"><span class="icon"></span>60dc332</a></td><td><code>removed empty class CenterSkewPolynomial_generic_dense</code>
</td></tr></table>
TicketarpitdmTue, 12 Jul 2016 18:22:24 GMT
https://trac.sagemath.org/ticket/13215#comment:47
https://trac.sagemath.org/ticket/13215#comment:47
<p>
The current ticket now contains only the basic framework for skew polynomials. All features related to finite fields, irreducibility, center and karatsuba multiplication will be put into a separate ticket. The aim is to fix the failed doctests in the <code>skew_polynomial_element.pyx</code> file and then refactor the code, improve/add documentation and tests, phase out "side".
</p>
TicketgitWed, 13 Jul 2016 06:02:58 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:48
https://trac.sagemath.org/ticket/13215#comment:48
<ul>
<li><strong>commit</strong>
changed from <em>60dc3329dcaf4ce971b68867999f02d71b2f989a</em> to <em>5b886b3be18614e3e41abd9a8eed11efefffff91</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=5b886b3be18614e3e41abd9a8eed11efefffff91"><span class="icon"></span>5b886b3</a></td><td><code>Fixed NotImplementedError and other standalone errors.</code>
</td></tr></table>
TicketjsrnWed, 13 Jul 2016 19:23:26 GMTbranch changed
https://trac.sagemath.org/ticket/13215#comment:49
https://trac.sagemath.org/ticket/13215#comment:49
<ul>
<li><strong>branch</strong>
changed from <em>u/arpitdm/skew_polynomials</em> to <em>u/jsrn/skew_polynomials</em>
</li>
</ul>
TicketjsrnWed, 13 Jul 2016 20:43:33 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:50
https://trac.sagemath.org/ticket/13215#comment:50
<ul>
<li><strong>commit</strong>
changed from <em>5b886b3be18614e3e41abd9a8eed11efefffff91</em> to <em>21be4d87405946f2f2ea3c415b4b2537b0ef6a80</em>
</li>
</ul>
<p>
Fixed some nasty errors, and some not so nasty ones.
</p>
<hr />
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=fa9f2a3241f23eab12484e459dc55fe709b0c8e2"><span class="icon"></span>fa9f2a3</a></td><td><code>Remove __cmp__</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=1061067feb1ba73149a7290cb35164a4d3256b4b"><span class="icon"></span>1061067</a></td><td><code>missing return statement</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=21be4d87405946f2f2ea3c415b4b2537b0ef6a80"><span class="icon"></span>21be4d8</a></td><td><code>Fixed some doctests</code>
</td></tr></table>
TicketarpitdmWed, 13 Jul 2016 21:42:05 GMTbranch changed
https://trac.sagemath.org/ticket/13215#comment:51
https://trac.sagemath.org/ticket/13215#comment:51
<ul>
<li><strong>branch</strong>
changed from <em>u/jsrn/skew_polynomials</em> to <em>u/arpitdm/skew_polynomials</em>
</li>
</ul>
TicketgitThu, 14 Jul 2016 13:53:22 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:52
https://trac.sagemath.org/ticket/13215#comment:52
<ul>
<li><strong>commit</strong>
changed from <em>21be4d87405946f2f2ea3c415b4b2537b0ef6a80</em> to <em>c65088d27f07256875c1aaccfd89264601b1e642</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=c0250fe09709af56694b7793e21c9d1112ed0df5"><span class="icon"></span>c0250fe</a></td><td><code>Edits to documentation, clean up code</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=3c758fea24e6a6f11eb7f85b607c76858f1dd3df"><span class="icon"></span>3c758fe</a></td><td><code>minor edit to a documentation</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=f0bd901d39f6c8e3b5126d2f72173a04efdc44bb"><span class="icon"></span>f0bd901</a></td><td><code>merging changes</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=0189fc2590cb3d8253c4e2380fc3c9ba67f00ffe"><span class="icon"></span>0189fc2</a></td><td><code>implemented special case for def is_unit when base ring is an integral domain</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=17ce5ecca59b7e8d0482357c693fd7f0839f1864"><span class="icon"></span>17ce5ec</a></td><td><code>Merge branch 'partially_refactored_skew_polynomials' into skew_polynomials</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=c65088d27f07256875c1aaccfd89264601b1e642"><span class="icon"></span>c65088d</a></td><td><code>fixed doctest that was mistakenly edited before</code>
</td></tr></table>
TicketgitThu, 14 Jul 2016 19:01:11 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:53
https://trac.sagemath.org/ticket/13215#comment:53
<ul>
<li><strong>commit</strong>
changed from <em>c65088d27f07256875c1aaccfd89264601b1e642</em> to <em>d7f51f9b2772e692b5914b71757d7fcc14d05e53</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=71e77deab5c0dfcc42531b23c3d928b6b808b2d5"><span class="icon"></span>71e77de</a></td><td><code>renamed lmonic as left_monic, rmonic as right_monic and removed def monic.</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=99664e62dafa51600750dce17f6a63ba9de53320"><span class="icon"></span>99664e6</a></td><td><code>renamed lquo_rem and rquo_rem as left_quo_rem and right_quo_rem. removed def quo_rem. split def is_divisible_by into def is_left_divisible_by and is_right_divisible_by. Same for def divides. replaced all instances in this file accordingly.</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=783f8df5766390f91adc9616049b8206b139df60"><span class="icon"></span>783f8df</a></td><td><code>renamed lxgcd and rxgcd as left_xgcd and right_xgcd. removed def xgcd.</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=2a4f087d596d413da41bf25a552aacb8d089e741"><span class="icon"></span>2a4f087</a></td><td><code>renamed lgcd and rgcd as left_gcd and right_gcd. removed def gcd.</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=cd44a5a4ddb047a03a12e7c99d8ca2a548e03e62"><span class="icon"></span>cd44a5a</a></td><td><code>fixed doctest errors from left_gcd. renamed llcm and rlcm as left_lcm and right_lcm. removed def lcm.</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=bc2a86318d8f70dc3f74902f5e19ffac4608102b"><span class="icon"></span>bc2a863</a></td><td><code>removed sig_on/sig_off from everywhere.</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=d7f51f9b2772e692b5914b71757d7fcc14d05e53"><span class="icon"></span>d7f51f9</a></td><td><code>small cleanup</code>
</td></tr></table>
TicketgitSat, 16 Jul 2016 22:44:41 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:54
https://trac.sagemath.org/ticket/13215#comment:54
<ul>
<li><strong>commit</strong>
changed from <em>d7f51f9b2772e692b5914b71757d7fcc14d05e53</em> to <em>98d357d6ba074e39cb28004edfda42b36136024f</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=f0f31b9144bc481a2ebc1026db015d0f53e3cab8"><span class="icon"></span>f0f31b9</a></td><td><code>removed def coeffs since it has been deprecated in polynomial_element.pyx. modified def coefficients to support return of only non-zero entries as well as all.</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=883146f2be3a46283875a6675ae517d33173eed5"><span class="icon"></span>883146f</a></td><td><code>added methods def reverse, def number_of_terms and def is_one.</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=cebe0b42cde63a0c2920a0506a5e024dc7f51f90"><span class="icon"></span>cebe0b4</a></td><td><code>added __copy__, evaluation of polynomial and is_constant methods.</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=98d357d6ba074e39cb28004edfda42b36136024f"><span class="icon"></span>98d357d</a></td><td><code>removed from pow method in Class SkewPolynomial_generic_dense. Added documentation and tests to all the remaining functions and classes.</code>
</td></tr></table>
TicketgitMon, 18 Jul 2016 21:03:19 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:55
https://trac.sagemath.org/ticket/13215#comment:55
<ul>
<li><strong>commit</strong>
changed from <em>98d357d6ba074e39cb28004edfda42b36136024f</em> to <em>e189fec13d005a7fba39a429876c501fe95c05da</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=49f26263f6a681f4ab70053083f45ebe85ee133b"><span class="icon"></span>49f2626</a></td><td><code>added a couple of tests. fixed typos in documentation.</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=56982a2cbf21440a0f272d92b26cd116e5fee811"><span class="icon"></span>56982a2</a></td><td><code>added documentation and tests. fixed doctests.</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=1808bb38901632f0f11039f921850d3d097106dd"><span class="icon"></span>1808bb3</a></td><td><code>small documentation for normalize function.</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=e189fec13d005a7fba39a429876c501fe95c05da"><span class="icon"></span>e189fec</a></td><td><code>removed side.py</code>
</td></tr></table>
TicketarpitdmMon, 18 Jul 2016 21:11:09 GMT
https://trac.sagemath.org/ticket/13215#comment:56
https://trac.sagemath.org/ticket/13215#comment:56
<p>
We planned on breaking up this skew polynomials ticket into five pieces namely:
</p>
<ol><li>Ticket A - The main framework. Skew polynomials, basic operations, coercions, skew polynomial rings, etc.
</li><li>Ticket B - Class SkewPolynomial_finite_field_dense, class SkewPolynomialRing_finite_field and related methods
</li><li>Ticket C - cdef class SkewPolynomial_finite_field_karatsuba and related methods
</li><li>Ticket D - class <a class="missing wiki">SectionSkewPolynomialCenterInjection?</a>, class <a class="missing wiki">SkewPolynomialCenterInjection?</a>, class <a class="missing wiki">CenterSkewPolynomialRing?</a> and related methods
</li><li>Ticket E - Irreducibility and Factoring related methods.
</li></ol><p>
The current ticket <a class="closed ticket" href="https://trac.sagemath.org/ticket/13215" title="enhancement: Skew polynomials (closed: fixed)">#13215</a> has been modified to cover Ticket A. I will post an update here with the links to the remaining tickets once they have been created. For now, based on discussions, the main framework has been refactored, appended to, edited, documented and tested. I'm now opening this ticket for review.
</p>
TicketarpitdmMon, 18 Jul 2016 21:11:38 GMTstatus changed
https://trac.sagemath.org/ticket/13215#comment:57
https://trac.sagemath.org/ticket/13215#comment:57
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
TicketarpitdmMon, 18 Jul 2016 21:13:38 GMTdescription changed
https://trac.sagemath.org/ticket/13215#comment:58
https://trac.sagemath.org/ticket/13215#comment:58
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/13215?action=diff&version=58">diff</a>)
</li>
</ul>
TicketdlucasTue, 19 Jul 2016 13:21:39 GMTstatus changed
https://trac.sagemath.org/ticket/13215#comment:59
https://trac.sagemath.org/ticket/13215#comment:59
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Hello,
</p>
<p>
I started reviewing your work. For now, I mostly read documentation and checked
formatting, without focusing on the code itself.
Here are my comments:
</p>
<h2 id="Genericcomments:">Generic comments:</h2>
<ul><li>For now, there's nothing related to skew polynomials in the index file, so it won't be possible
for the user to access the documentation through the online reference manual.
Can you please add a dedicated section in the related index file? See <code>src/doc/en/reference/polynomial_rings/index.rst</code>.
</li></ul><ul><li>I would also use the experimental decorator here: the code in itself seems stable, but it
will allow us to make later name changes (for instance) without putting deprecation warnings
all over the place. In particular, I have in mind later implementations of sparse rings and
multivariate rings, which might trigger some renaming in the classes implemented here.
</li></ul><h2 id="skew_polynomial_ring_constructor.py">skew_polynomial_ring_constructor.py</h2>
<ul><li>I might be wrong, but it does not seem to have any reason to get both input <code>name</code> and <code>names</code>.
The argument in the <code>NOTE</code> block sounds weird to me, especially when reading the code: it seems
to be some kind of argument for later enhancements (multivariate skew polynomial rings), as for now
it does not work if the list contains more than one element. Furthermore, the input <code>names</code> is
only used to set <code>name</code> if the latter is <code>None</code>, and dropped just after.
And in any case, I think it's better to use only one input and use type checks.
I propose to remove <code>names</code>, rename <code>name</code> into <code>variable_names</code>, and use sanity checks: if it's
a string, or a list with a single string, use it, otherwise return an appropriate error.
I also suggest to add a line in the note block to say that multivariate rings are not
supported (it's only detailed in tests for now).
</li><li>Can you please explicit what <code>LHS</code>/<code>RHS</code> means?
</li><li>There's an option <code>sparse=False</code> which, if set to <code>True</code>, returns a <code>NotImplementedError</code>.
I suggest to remove it for now in order to not confuse the user, and reinstate it later
when it will be fully implemented. In this case, also remove all references to this feature
in the documentation.
</li><li><code>INPUT</code> block could be enhanced: I would rather explain what's <code>name</code> (or <code>variable_names</code> if
you agree with my comment above) instead of just saying "a string". I would also rename <code>sigma</code>
to something more indicative. What about <code>base_ring_endomorphism</code>?
</li><li>As I suggest to remove some not implemented but planned features, it might be a good
idea to add a <code>TODO</code> block as a reminder of these features (sparse rings, multivariate rings).
</li><li>Module title says "constructorS for skew polynomial rings", while there's only one method
in this module. <code></quibbling></code>
</li><li>Module documentation itself is not great. I think it might be a good idea to add a few
links to <code>skew_polynomial_ring.py</code>, and even to give a small definition of a skew polynomial ring
in this module.
</li></ul><h2 id="skew_polynomial_ring.py">skew_polynomial_ring.py</h2>
<ul><li>The short description of this module:
"Sage implements dense skew univariate polynomials over commutative rings.",
seems unhelpful. I'd rather replace it by something explaining the purpose of this
module.
</li><li>I think the definition can be enhanced a bit: for instance, you find references to "twisting"
all over the examples, but it's not defined everywhere. Reading what's in the <code>DEFINITION</code> block
should be enough to understand the terminology used in the module, even if you're not familiar with
skew polynomials at all.
</li><li>Same remark as above regarding <code>LHS</code>/<code>RHS</code>.
</li><li>Some imports are not necessary anymore, e.g. <code>import sage.misc.latex as latex</code>.
</li><li>Some methods here do not follow the mandatory documentation template.
For instance, <code>__init__</code> does not have a short description, nor an <code>INPUT</code> block...
</li><li>There are a few methods in this file which are commented. If these are no longer useful,
I suggest to remove them instead of commeting them.
</li><li>In some methods, documentation is not really helpful: see <code>twist_map</code> for instance.
If says "Return the twist map [...]" which is not very informative...
</li><li>While I'm talking about it, would it be possible to explain why this method might fail
if you pass a negative number as input? The sentence "Sometimes it succeeds" seems to imply
it's more or less random, but if you could investigate this a bit it would lead to a more
satisfying documentation.
</li></ul><h2 id="skew_polynomial_element.py">skew_polynomial_element.py</h2>
<ul><li>Same remark as above regarding the definition of a skew polynomial ring.
</li><li>Same remark as above regarding imported packages.
</li><li>Same remark as above regarding informativeness of docstrings for some methods.
</li><li>Same remark as above regarding documentation itself: a few methods (e.g. <code>conjugate</code>) do
not have a short description, while <code>make_generic_skew_polynomial</code> does not have documentation at all!
</li></ul><p>
Best,
</p>
<p>
David
</p>
TickettscrimTue, 19 Jul 2016 13:57:29 GMT
https://trac.sagemath.org/ticket/13215#comment:60
https://trac.sagemath.org/ticket/13215#comment:60
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:59" title="Comment 59">dlucas</a>:
</p>
<blockquote class="citation">
<ul><li>I would also use the experimental decorator here: the code in itself seems stable, but it
will allow us to make later name changes (for instance) without putting deprecation warnings
all over the place. In particular, I have in mind later implementations of sparse rings and
multivariate rings, which might trigger some renaming in the classes implemented here.
</li></ul></blockquote>
<p>
It is okay to change class names as long as the public API doesn't change (although this can entail unpickling issues in either case). I would instead future-proof this by
</p>
<blockquote class="citation">
<ul><li>Can you please explicit what <code>LHS</code>/<code>RHS</code> means?
</li></ul></blockquote>
<p>
I think these are such common abbreviations for Left(Right)-Hand Side that being explicit about them will add some clutter.
</p>
<blockquote class="citation">
<ul><li>There's an option <code>sparse=False</code> which, if set to <code>True</code>, returns a <code>NotImplementedError</code>.
I suggest to remove it for now in order to not confuse the user, and reinstate it later
when it will be fully implemented. In this case, also remove all references to this feature
in the documentation.
</li><li>As I suggest to remove some not implemented but planned features, it might be a good
idea to add a <code>TODO</code> block as a reminder of these features (sparse rings, multivariate rings).
</li></ul></blockquote>
<p>
I disagree with these comments. This is documenting an upcoming feature, it helps keeps the API consistent with future plans and other parts of Sage, and tells someone reading the code what is currently not implemented.
</p>
<blockquote class="citation">
<ul><li>Some methods here do not follow the mandatory documentation template.
For instance, <code>__init__</code> does not have a short description, nor an <code>INPUT</code> block...
</li></ul></blockquote>
<p>
I treat this more as a strong guideline than a hard rule, but with that being said, anything more non-trivial should at least document its inputs (e.g., the <code>__init__</code> would be useful).
</p>
<p>
I agree with the rest of David's comments.
</p>
<p>
Also, from a cursory glance, there seems to be a lot of duplication with the current implementations of polynomials (i.e., the elements). I feel there could be a lot of simplification of the code by either subclassing or creating a mix-in (cython) class that overrides the <code>_mul_</code> of polynomials. Is there a reason why you didn't do this? This might entail doing some refactoring/abstracting of the polynomial code though, but it should make the implementation of skew polynomials a lot easier (and bug proof).
</p>
TickettscrimTue, 19 Jul 2016 13:57:58 GMTwork_issues deleted
https://trac.sagemath.org/ticket/13215#comment:61
https://trac.sagemath.org/ticket/13215#comment:61
<ul>
<li><strong>work_issues</strong>
<em>see comment #25</em> deleted
</li>
</ul>
<p>
One last thing, make sure all functions have doctests.
</p>
TicketarpitdmTue, 19 Jul 2016 20:06:14 GMT
https://trac.sagemath.org/ticket/13215#comment:62
https://trac.sagemath.org/ticket/13215#comment:62
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:59" title="Comment 59">dlucas</a>:
</p>
<blockquote class="citation">
<ul><li>For now, there's nothing related to skew polynomials in the index file, so it won't be possible
for the user to access the documentation through the online reference manual.
Can you please add a dedicated section in the related index file? See <code>src/doc/en/reference/polynomial_rings/index.rst</code>.
</li></ul></blockquote>
<p>
I read up a little on Sphinx documentation and from what I understand, I need to add the following in the file you mentioned, right?
</p>
<pre class="wiki">@sage.misc.superseded.experimental(trac_number=13215)
Skew Polynomials
----------------
.. toctree::
:maxdepth: 2
sage/rings/polynomial/skew_polynomial_element
sage/rings/polynomial/skew_polynomial_ring_constructor
sage/rings/polynomial/skew_polynomial_ring
</pre><blockquote class="citation">
<ul><li>I would also use the experimental decorator here: the code in itself seems stable, but it
will allow us to make later name changes (for instance) without putting deprecation warnings
all over the place. In particular, I have in mind later implementations of sparse rings and
multivariate rings, which might trigger some renaming in the classes implemented here.
</li></ul></blockquote>
<p>
Placing the experimental warning (like I did above) is not the right way to do it because it does not throw an error. Do I instead place this line at the start of every class in all the three files instead?
</p>
<blockquote class="citation">
<h2 id="skew_polynomial_ring_constructor.py">skew_polynomial_ring_constructor.py</h2>
<ul><li>I might be wrong, but it does not seem to have any reason to get both input <code>name</code> and <code>names</code>.
The argument in the <code>NOTE</code> block sounds weird to me, especially when reading the code: it seems
to be some kind of argument for later enhancements (multivariate skew polynomial rings), as for now
it does not work if the list contains more than one element. Furthermore, the input <code>names</code> is
only used to set <code>name</code> if the latter is <code>None</code>, and dropped just after.
And in any case, I think it's better to use only one input and use type checks.
I propose to remove <code>names</code>, rename <code>name</code> into <code>variable_names</code>, and use sanity checks: if it's
a string, or a list with a single string, use it, otherwise return an appropriate error.
I also suggest to add a line in the note block to say that multivariate rings are not
supported (it's only detailed in tests for now).
</li></ul></blockquote>
<p>
I agree. <code>names</code> and <code>name</code> are both not required and <code>variable_names</code> along with tests to check type of the input is cleaner. I've made this change.
</p>
<blockquote class="citation">
<ul><li>Can you please explicit what <code>LHS</code>/<code>RHS</code> means?
</li></ul></blockquote>
<p>
Like @tscrim mentions, I think LHS/RHS are extremely common terms that are used almost everywhere and perhaps don't need explicit description.
</p>
<blockquote class="citation">
<ul><li>There's an option <code>sparse=False</code> which, if set to <code>True</code>, returns a <code>NotImplementedError</code>.
I suggest to remove it for now in order to not confuse the user, and reinstate it later
when it will be fully implemented. In this case, also remove all references to this feature
in the documentation.
</li></ul></blockquote>
<p>
There are a lot of places where a <a class="missing wiki">NotImplementedError?</a> is raised and not just in this ticket but in other polynomial files as well. I think it makes more sense to have them if they are already there. However, there is then the inconsistency in that derivations in skew polynomials are not implemented and are marked as <code>todo</code> as is the multivariate case in some places. Still, I argue for keeping sparse as it is since it is integrated in the framework already unlike the other two. And with the experimental warning showing up, changing this will not create too much trouble.
</p>
<blockquote class="citation">
<ul><li><code>INPUT</code> block could be enhanced: I would rather explain what's <code>name</code> (or <code>variable_names</code> if
you agree with my comment above) instead of just saying "a string". I would also rename <code>sigma</code>
to something more indicative. What about <code>base_ring_endomorphism</code>?
</li></ul></blockquote>
<p>
An endomorphism that admits an inverse is an automorphism and skew polynomial rings AFAIK are based on automorphisms. Please correct me if I'm mistaken in this. If so, the places where it says that the twist map is not invertible, maybe should say that twist map should be invertible. And accordingly, the name can be changed to <code>base_ring_automorphism</code>.
</p>
<blockquote class="citation">
<ul><li>Module documentation itself is not great. I think it might be a good idea to add a few
links to <code>skew_polynomial_ring.py</code>, and even to give a small definition of a skew polynomial ring
in this module.
</li></ul></blockquote>
<p>
Understood. I've made changes and expanded on the definitions and examples from other files. Same goes for all other notes (below) about improving explanations.
</p>
<blockquote class="citation">
<ul><li>Some imports are not necessary anymore, e.g. <code>import sage.misc.latex as latex</code>.
</li></ul></blockquote>
<p>
I tried to do that with the ones I thought were not needed/deprecated. I've obviously missed a few. I'll go over this once again.
</p>
<blockquote class="citation">
<ul><li>Some methods here do not follow the mandatory documentation template.
For instance, <code>__init__</code> does not have a short description, nor an <code>INPUT</code> block...
</li></ul></blockquote>
<p>
Is there a reference doc or something where I can go through the mandatory documentation template? Or is it that Description, Input, Output, Examples, Tests - should these be present for every method? A lot of places, it doesn't make sense to have all of this.
</p>
<blockquote class="citation">
<ul><li>In some methods, documentation is not really helpful: see <code>twist_map</code> for instance.
not have a short description, while <code>make_generic_skew_polynomial</code> does not have documentation at all!
</li></ul></blockquote>
<p>
Oh right! I mentioned this in the email thread with Johan except that I somehow messed up the name. I wasn't sure of what this method was doing.
</p>
TicketdlucasWed, 20 Jul 2016 07:49:01 GMT
https://trac.sagemath.org/ticket/13215#comment:63
https://trac.sagemath.org/ticket/13215#comment:63
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:62" title="Comment 62">arpitdm</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:59" title="Comment 59">dlucas</a>:
</p>
<blockquote class="citation">
<ul><li>For now, there's nothing related to skew polynomials in the index file, so it won't be possible
for the user to access the documentation through the online reference manual.
Can you please add a dedicated section in the related index file? See <code>src/doc/en/reference/polynomial_rings/index.rst</code>.
</li></ul></blockquote>
<p>
I read up a little on Sphinx documentation and from what I understand, I need to add the following in the file you mentioned, right?
</p>
<pre class="wiki">@sage.misc.superseded.experimental(trac_number=13215)
Skew Polynomials
----------------
.. toctree::
:maxdepth: 2
sage/rings/polynomial/skew_polynomial_element
sage/rings/polynomial/skew_polynomial_ring_constructor
sage/rings/polynomial/skew_polynomial_ring
</pre></blockquote>
<p>
Yup! I you're not sure of what you're doing, here's a way to test it:
run <code>sage --docbuild src/doc/en/reference/polynomial_rings html</code>, it will compile the
doc and give you an address. Copy-paste this address in your browser, and click on "index"
in the page which opens. If what you added in <code>index.rst</code> works, you should see it here (and
you'll be able to check its formatting as well).
<br /><br />
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>I would also use the experimental decorator here: the code in itself seems stable, but it
will allow us to make later name changes (for instance) without putting deprecation warnings
all over the place. In particular, I have in mind later implementations of sparse rings and
multivariate rings, which might trigger some renaming in the classes implemented here.
</li></ul></blockquote>
<p>
Placing the experimental warning (like I did above) is not the right way to do it because it does not throw an error. Do I instead place this line at the start of every class in all the three files instead?
</p>
</blockquote>
<p>
Mmh, I reconsidered my position on this thanks to Travis' comment: as we'll not change the public API (which is the method in <code>skew_polynomial_ring_constructor.py</code>), I don't think it's necessary to
put experimental warnings everywhere, as long as we're future proof as Travis suggests.
<br /><br />
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>Can you please explicit what <code>LHS</code>/<code>RHS</code> means?
</li></ul></blockquote>
<p>
Like @tscrim mentions, I think LHS/RHS are extremely common terms that are used almost everywhere and perhaps don't need explicit description.
</p>
</blockquote>
<p>
Yes, you're both right, let's drop that and keep <code>LHS</code>/<code>RHS</code>.
<br /><br />
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>There's an option <code>sparse=False</code> which, if set to <code>True</code>, returns a <code>NotImplementedError</code>.
I suggest to remove it for now in order to not confuse the user, and reinstate it later
when it will be fully implemented. In this case, also remove all references to this feature
in the documentation.
</li></ul></blockquote>
<p>
There are a lot of places where a <a class="missing wiki">NotImplementedError?</a> is raised and not just in this ticket but in other polynomial files as well. I think it makes more sense to have them if they are already there. However, there is then the inconsistency in that derivations in skew polynomials are not implemented and are marked as <code>todo</code> as is the multivariate case in some places. Still, I argue for keeping sparse as it is since it is integrated in the framework already unlike the other two. And with the experimental warning showing up, changing this will not create too much trouble.
</p>
</blockquote>
<p>
Yes, thinking again about it, I think you're both right. It's better to be future proof, and if
it's used elsewhere in <code>polynomials</code>, there's definitely no trouble in doing so.
<br /><br />
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li><code>INPUT</code> block could be enhanced: I would rather explain what's <code>name</code> (or <code>variable_names</code> if
you agree with my comment above) instead of just saying "a string". I would also rename <code>sigma</code>
to something more indicative. What about <code>base_ring_endomorphism</code>?
</li></ul></blockquote>
<p>
An endomorphism that admits an inverse is an automorphism and skew polynomial rings AFAIK are based on automorphisms. Please correct me if I'm mistaken in this. If so, the places where it says that the twist map is not invertible, maybe should say that twist map should be invertible. And accordingly, the name can be changed to <code>base_ring_automorphism</code>.
</p>
</blockquote>
<p>
Err, yes my bad. <code>base_ring_automorphism</code> sounds good <code>:)</code>
<br /><br />
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>Some imports are not necessary anymore, e.g. <code>import sage.misc.latex as latex</code>.
</li></ul></blockquote>
<p>
I tried to do that with the ones I thought were not needed/deprecated. I've obviously missed a few. I'll go over this once again.
</p>
</blockquote>
<p>
Don't worry, it's quite tricky.
<br /><br />
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>Some methods here do not follow the mandatory documentation template.
For instance, <code>__init__</code> does not have a short description, nor an <code>INPUT</code> block...
</li></ul></blockquote>
<p>
Is there a reference doc or something where I can go through the mandatory documentation template? Or is it that Description, Input, Output, Examples, Tests - should these be present for every method? A lot of places, it doesn't make sense to have all of this.
</p>
</blockquote>
<p>
You can read this <a class="ext-link" href="http://doc.sagemath.org/html/en/developer/coding_basics.html#documentation-strings"><span class="icon"></span>tutorial</a> for Sage developers.
But I agree this is not strong rules: while I do follow the "short description, jump line, optional
long description" for my methods, I only write an <code>OUTPUT</code> block when the output is not obvious from reading the short description. I always write an <code>INPUT</code> block when there's input arguments, as I
think that even if the names of the input arguments seems obvious, it's always nice to have an
explicit definition. <code>EXAMPLES</code> are of course mandatory. <code>TESTS</code> are not, I think you should only
use them when you want to run a few more tests which do not really make sense for the user. For instance, I use <code>TESTS</code> to doctest sanity checks. It's nice to add a doctest to ensure everything is
sanitized, but it's of little interest for the user.
<br /><br />
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>In some methods, documentation is not really helpful: see <code>twist_map</code> for instance.
not have a short description, while <code>make_generic_skew_polynomial</code> does not have documentation at all!
</li></ul></blockquote>
<p>
Oh right! I mentioned this in the email thread with Johan except that I somehow messed up the name. I wasn't sure of what this method was doing.
</p>
</blockquote>
<p>
Oh ok! I'll read it and try to understand what it does.
</p>
<p>
Best,
</p>
<p>
David
</p>
TicketarpitdmThu, 21 Jul 2016 00:19:55 GMT
https://trac.sagemath.org/ticket/13215#comment:64
https://trac.sagemath.org/ticket/13215#comment:64
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:63" title="Comment 63">dlucas</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:62" title="Comment 62">arpitdm</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:59" title="Comment 59">dlucas</a>:
</p>
<blockquote class="citation">
<ul><li>For now, there's nothing related to skew polynomials in the index file, so it won't be possible
for the user to access the documentation through the online reference manual.
Can you please add a dedicated section in the related index file? See <code>src/doc/en/reference/polynomial_rings/index.rst</code>.
</li></ul></blockquote>
<p>
I read up a little on Sphinx documentation and from what I understand, I need to add the following in the file you mentioned, right?
</p>
<pre class="wiki">@sage.misc.superseded.experimental(trac_number=13215)
Skew Polynomials
----------------
.. toctree::
:maxdepth: 2
sage/rings/polynomial/skew_polynomial_element
sage/rings/polynomial/skew_polynomial_ring_constructor
sage/rings/polynomial/skew_polynomial_ring
</pre></blockquote>
<p>
Yup! I you're not sure of what you're doing, here's a way to test it:
run <code>sage --docbuild src/doc/en/reference/polynomial_rings html</code>, it will compile the
doc and give you an address. Copy-paste this address in your browser, and click on "index"
in the page which opens. If what you added in <code>index.rst</code> works, you should see it here (and
you'll be able to check its formatting as well).
</p>
</blockquote>
<p>
That command you mentioned didn't work. I get the following error message.
</p>
<pre class="wiki">'src/doc/en/reference/polynomial_rings' is not a recognized document. Type 'sage --docbuild -D' for a list
of documents, or 'sage --docbuild --help' for more help.
</pre><p>
I've made the other changes according to the discussions above.
</p>
TicketjsrnThu, 21 Jul 2016 02:00:56 GMT
https://trac.sagemath.org/ticket/13215#comment:65
https://trac.sagemath.org/ticket/13215#comment:65
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:60" title="Comment 60">tscrim</a>:
</p>
<blockquote class="citation">
<p>
It is okay to change class names as long as the public API doesn't change (although this can entail unpickling issues in either case). I would instead future-proof this by
</p>
</blockquote>
<p>
Was there supposed to be some ending of this sentence?
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>There's an option <code>sparse=False</code> which, if set to <code>True</code>, returns a <code>NotImplementedError</code>.
I suggest to remove it for now in order to not confuse the user, and reinstate it later
when it will be fully implemented. In this case, also remove all references to this feature
in the documentation.
</li><li>As I suggest to remove some not implemented but planned features, it might be a good
idea to add a <code>TODO</code> block as a reminder of these features (sparse rings, multivariate rings).
</li></ul></blockquote>
<p>
I disagree with these comments. This is documenting an upcoming feature, it helps keeps the API consistent with future plans and other parts of Sage, and tells someone reading the code what is currently not implemented.
</p>
</blockquote>
<p>
This is debatable: sparse skew polynomials are not very useful whenever there is a derivation (since sparsity is not retained well across operations), and it's not very clear what a multivariate skew ring is. The most natural generalisation of this structure is Ore polynomials, but that would likely be a separate class. I don't think it makes sense to leave implementation artifacts that will, most likely, never lead to real implementations. In the odd case that they *would* lead to implementations, we could reinstate them with no penalty.
</p>
<blockquote class="citation">
<p>
Also, from a cursory glance, there seems to be a lot of duplication with the current implementations of polynomials (i.e., the elements). I feel there could be a lot of simplification of the code by either subclassing or creating a mix-in (cython) class that overrides the <code>_mul_</code> of polynomials. Is there a reason why you didn't do this? This might entail doing some refactoring/abstracting of the polynomial code though, but it should make the implementation of skew polynomials a lot easier (and bug proof).
</p>
</blockquote>
<p>
That's arguably true. To be fair: Xavier Caruso made the code, and Arpit just tried to get it into shape with less than full-rewrite-workload. That said, I would be concerned about subclassing commutative classes that commutative-only code creeps into the skew polynomials: for instance if someone later on adds a method to the parent class and forgets to explicitly overwrite or remove this method in the skew polynomial class. It's really only all the basics that are shared btw commutative/non-commutative: all the substantial and complex properties are not shared at all.
</p>
TicketjsrnThu, 21 Jul 2016 02:03:36 GMT
https://trac.sagemath.org/ticket/13215#comment:66
https://trac.sagemath.org/ticket/13215#comment:66
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:65" title="Comment 65">jsrn</a>:
</p>
<blockquote class="citation">
<p>
This is debatable: sparse skew polynomials are not very useful whenever there is a derivation (since sparsity is not retained well across operations),
</p>
</blockquote>
<p>
Oh, I just remembered that this patch doesn't provide skew polynomials with derivations :-D So then this comment might be somewhat moot ;-)
</p>
<blockquote class="citation">
<p>
The most natural generalisation of this structure is Ore polynomials,
</p>
</blockquote>
<p>
One would perhaps add skew polynomials with derivations before full-fledged Ore rings, though :-)
</p>
<p>
Best,
Johan
</p>
TickettscrimThu, 21 Jul 2016 02:37:16 GMT
https://trac.sagemath.org/ticket/13215#comment:67
https://trac.sagemath.org/ticket/13215#comment:67
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:65" title="Comment 65">jsrn</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:60" title="Comment 60">tscrim</a>:
</p>
<blockquote class="citation">
<p>
It is okay to change class names as long as the public API doesn't change (although this can entail unpickling issues in either case). I would instead future-proof this by
</p>
</blockquote>
<p>
Was there supposed to be some ending of this sentence?
</p>
</blockquote>
<p>
That must of gotten lost when I had connectivity issues. This was suppose to be "by using a consistent API".
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
Also, from a cursory glance, there seems to be a lot of duplication with the current implementations of polynomials (i.e., the elements). I feel there could be a lot of simplification of the code by either subclassing or creating a mix-in (cython) class that overrides the <code>_mul_</code> of polynomials. Is there a reason why you didn't do this? This might entail doing some refactoring/abstracting of the polynomial code though, but it should make the implementation of skew polynomials a lot easier (and bug proof).
</p>
</blockquote>
<p>
That's arguably true. To be fair: Xavier Caruso made the code, and Arpit just tried to get it into shape with less than full-rewrite-workload.
</p>
</blockquote>
<p>
I thank them both for working on this and pushing it forward.
</p>
<blockquote class="citation">
<p>
That said, I would be concerned about subclassing commutative classes that commutative-only code creeps into the skew polynomials: for instance if someone later on adds a method to the parent class and forgets to explicitly overwrite or remove this method in the skew polynomial class. It's really only all the basics that are shared btw commutative/non-commutative: all the substantial and complex properties are not shared at all.
</p>
</blockquote>
<p>
I don't want to diminish their work, but sometimes after having the code there and looking ahead, major refactoring can be needed. However, there would likely need to be refactoring of the usual commutative polynomials to address the issue you brought up. There could also be ways to factor out common code from other algebras across Sage. So in short, what I'm suggesting is to have a common abstract base class for both for the basic/core functionality. If there is only a little overlap (I really haven't looked), then we can just continue in our current fashion.
</p>
TicketjsrnThu, 21 Jul 2016 02:50:56 GMT
https://trac.sagemath.org/ticket/13215#comment:68
https://trac.sagemath.org/ticket/13215#comment:68
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:67" title="Comment 67">tscrim</a>:
</p>
<blockquote class="citation">
<p>
That must of gotten lost when I had connectivity issues. This was suppose to be "by using a consistent API".
</p>
</blockquote>
<p>
Ah ok. You're right: aim for a good API. I think that the experimental decorator is still a good idea for such a big, brand-new feature, though. That what it was introduced for.
</p>
<blockquote class="citation">
<p>
I don't want to diminish their work, but sometimes after having the code there and looking ahead, major refactoring can be needed. However, there would likely need to be refactoring of the usual commutative polynomials to address the issue you brought up. There could also be ways to factor out common code from other algebras across Sage. So in short, what I'm suggesting is to have a common abstract base class for both for the basic/core functionality. If there is only a little overlap (I really haven't looked), then we can just continue in our current fashion.
</p>
</blockquote>
<p>
I agree that this would be nice and should be looked at. However, can I suggest
that this is for another ticket (cf. Reviewer's Checklist <a class="missing wiki">The perfect is the enemy of the good?</a><a class="ext-link" href="http://doc.sagemath.org/html/en/developer/reviewer_checklist.html"><span class="icon"></span>http://doc.sagemath.org/html/en/developer/reviewer_checklist.html</a>).
</p>
<p>
That way, improvements could progress on both ends (improving code sharing between the polynomial types, while improving skew poly functionality, and the coding theory constructions that we will build using skew polynomials).
</p>
TickettscrimThu, 21 Jul 2016 03:12:17 GMT
https://trac.sagemath.org/ticket/13215#comment:69
https://trac.sagemath.org/ticket/13215#comment:69
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:68" title="Comment 68">jsrn</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:67" title="Comment 67">tscrim</a>:
</p>
<blockquote class="citation">
<p>
That must of gotten lost when I had connectivity issues. This was suppose to be "by using a consistent API".
</p>
</blockquote>
<p>
Ah ok. You're right: aim for a good API. I think that the experimental decorator is still a good idea for such a big, brand-new feature, though. That what it was introduced for.
</p>
</blockquote>
<p>
In this case we have very similar features that we have had for a long time, in addition to a certain amount of stability with the code. By that, I think we shouldn't scare users with such a warning.
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
I don't want to diminish their work, but sometimes after having the code there and looking ahead, major refactoring can be needed. However, there would likely need to be refactoring of the usual commutative polynomials to address the issue you brought up. There could also be ways to factor out common code from other algebras across Sage. So in short, what I'm suggesting is to have a common abstract base class for both for the basic/core functionality. If there is only a little overlap (I really haven't looked), then we can just continue in our current fashion.
</p>
</blockquote>
<p>
I agree that this would be nice and should be looked at. However, can I suggest
that this is for another ticket (cf. Reviewer's Checklist <a class="missing wiki">The perfect is the enemy of the good?</a><a class="ext-link" href="http://doc.sagemath.org/html/en/developer/reviewer_checklist.html"><span class="icon"></span>http://doc.sagemath.org/html/en/developer/reviewer_checklist.html</a>).
</p>
</blockquote>
<p>
I'm not asking for perfection. If we do decide to subclass, then it just makes it somewhat harder to refactor once this is included. Just to be clear, I am just raising the question, not making the refactoring a requirement.
</p>
<blockquote class="citation">
<p>
That way, improvements could progress on both ends (improving code sharing between the polynomial types, while improving skew poly functionality, and the coding theory constructions that we will build using skew polynomials).
</p>
</blockquote>
<p>
If only I had time... :P
</p>
TicketdlucasThu, 21 Jul 2016 08:09:35 GMT
https://trac.sagemath.org/ticket/13215#comment:70
https://trac.sagemath.org/ticket/13215#comment:70
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:64" title="Comment 64">arpitdm</a>:
</p>
<blockquote class="citation">
<p>
That command you mentioned didn't work. I get the following error message.
</p>
<pre class="wiki">'src/doc/en/reference/polynomial_rings' is not a recognized document. Type 'sage --docbuild -D' for a list
of documents, or 'sage --docbuild --help' for more help.
</pre><p>
I've made the other changes according to the discussions above.
</p>
</blockquote>
<p>
Oh, my mistake, <code>sage --docbuild reference/polynomial_rings html</code> is enough... By the way, don't forget to rebuild before compiling documentation. If you don't, changes you made to the docstrings/tests since your last rebuild will not be considered.
</p>
<p>
Best,
</p>
<p>
David
</p>
TicketarpitdmThu, 21 Jul 2016 11:19:33 GMT
https://trac.sagemath.org/ticket/13215#comment:71
https://trac.sagemath.org/ticket/13215#comment:71
<p>
Hi David,
</p>
<p>
The <code>name</code> and <code>names</code> that we had agreed upon changing to <code>variable_names</code> is giving a problem I did not notice until I was testing stuff today. I think the reason they exist is not just to facilitate multi-variate rings but also to support the diamond bracket notation. <code>S.<x> = SkewPolynomialRing(R, sigma)</code> and without it gives <a class="missing wiki">TypeError?</a> (unexpected keyword argument). Also, this is a syntax present in other ring constructors in Sage. And while we don't support multivariate rings for Skew Polynomials, perhaps this feature might be added later on and we do need the diamond notation. Is there an alternate way in Sage to capture the diamond bracket or should we revert back to the original?
</p>
TicketdlucasThu, 21 Jul 2016 12:08:14 GMT
https://trac.sagemath.org/ticket/13215#comment:72
https://trac.sagemath.org/ticket/13215#comment:72
<p>
Hello,
</p>
<p>
Oh, ok I was not aware of that. I tested it on my side, and indeed whenever you use diamond bracket, it expects an argument called <code>names</code>.
Xavier's comments on the <code>NOTE</code> block makes sense now.
</p>
<p>
In that case, what about removing the argument <code>name</code> and keep only <code>names</code>?
Then, through input sanitization, allow only string, list containing one string, fail with a specific error if one passes a list with several strings in it explaining multivariate ring is not yet supported, and fail with a generic error message, e.g. "You can only pass a string, or a list of string for input <code>names</code>" otherwise.
I think it does the trick.
</p>
<p>
David
</p>
TicketjsrnThu, 21 Jul 2016 12:41:41 GMT
https://trac.sagemath.org/ticket/13215#comment:73
https://trac.sagemath.org/ticket/13215#comment:73
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:69" title="Comment 69">tscrim</a>:
</p>
<blockquote class="citation">
<p>
In this case we have very similar features that we have had for a long time, in addition to a certain amount of stability with the code. By that, I think we shouldn't scare users with such a warning.
</p>
</blockquote>
<p>
You have a point. The names and conventions are unlikely to change within the
limited scope of this ticket. For the follow-up tickets containing the rest of
Caruso's code the experimental decorator could be considered.
</p>
<blockquote class="citation">
<p>
I'm not asking for perfection. If we do decide to subclass, then it just makes it somewhat harder to refactor once this is included. Just to be clear, I am just raising the question, not making the refactoring a requirement.
</p>
</blockquote>
<p>
I don't think it makes it harder to refactor later on than now: it's exactly the same work, since the full implementation of skew polynomials has already been written, doctested etc. That's one reason I would suggest leaving it for a future ticket.
</p>
TickettscrimThu, 21 Jul 2016 13:50:52 GMT
https://trac.sagemath.org/ticket/13215#comment:74
https://trac.sagemath.org/ticket/13215#comment:74
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:73" title="Comment 73">jsrn</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:69" title="Comment 69">tscrim</a>:
</p>
<blockquote class="citation">
<p>
I'm not asking for perfection. If we do decide to subclass, then it just makes it somewhat harder to refactor once this is included. Just to be clear, I am just raising the question, not making the refactoring a requirement.
</p>
</blockquote>
<p>
I don't think it makes it harder to refactor later on than now: it's exactly the same work, since the full implementation of skew polynomials has already been written, doctested etc. That's one reason I would suggest leaving it for a future ticket.
</p>
</blockquote>
<p>
It just means we have to deal with getting old pickles to work, which might be non-trivial (granted, Sage is not always successful with this).
</p>
TickettscrimThu, 21 Jul 2016 14:03:16 GMT
https://trac.sagemath.org/ticket/13215#comment:75
https://trac.sagemath.org/ticket/13215#comment:75
<p>
You do want to use <code>names</code>:
</p>
<pre class="wiki">sage: preparse('R.<x> = PolynomialRing(ZZ)')
"R = PolynomialRing(ZZ, names=('x',)); (x,) = R._first_ngens(1)"
</pre><p>
Some notes on the code from a skim through:
</p>
<ul><li>You seem to be using the exact same format of polynomials, which contains a lot of old code. For example, don't implement a <code>_repr_()</code> that just calls (a <em>Python</em> function) <code>_repr()</code>. This is useless (even in the usual polynomial ring code it seems); instead just implement <code>_repr_()</code>. Similarly, there are a number of methods you could simply <code>cpdef</code>.
</li><li>Kill with fire and holy water <code>is_*</code> methods. They just trivially call <code>isinstance</code>.
</li><li>You can use
<pre class="wiki">self._no_generic_basering_coercion = True
</pre>to avoid constructing the coercion from the base ring when initializing the skew polynomial ring.
</li><li><code>_cmp_</code> is planned to be removed. Use <code>_richcmp_</code> instead.
</li><li>You should lazy import the ring constructor.
</li><li>You are double importing things in the element class; only keep the <code>cimport</code>'s.
</li></ul>
TicketjsrnThu, 21 Jul 2016 19:11:41 GMT
https://trac.sagemath.org/ticket/13215#comment:76
https://trac.sagemath.org/ticket/13215#comment:76
<p>
Hi Travis,
</p>
<p>
You seem to understand these things quite well, and we had quite some problems
with getting this code to work, so could you please elaborate on the following
things:
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:75" title="Comment 75">tscrim</a>:
</p>
<blockquote class="citation">
<ul><li>You seem to be using the exact same format of polynomials, which contains a lot of old code. For example, don't implement a <code>_repr_()</code> that just calls (a <em>Python</em> function) <code>_repr()</code>. This is useless (even in the usual polynomial ring code it seems); instead just implement <code>_repr_()</code>. Similarly, there are a number of methods you could simply <code>cpdef</code>.
</li></ul></blockquote>
<p>
That would be great. Can you give us the full list of the methods for which we can do this, or point us to how we can figure it out?
</p>
<blockquote class="citation">
<ul><li>You can use
<pre class="wiki">self._no_generic_basering_coercion = True
</pre>to avoid constructing the coercion from the base ring when initializing the skew polynomial ring.
</li></ul></blockquote>
<p>
To be precise, do you mean that can insert the above line in <code>SkewPolynomialRing_general.__init__</code> just before the call to <code>Algebra.__init__</code>, and then remove the call to <code>self._unset_coercions_used()</code>? Or is there more to it?
</p>
<blockquote class="citation">
<ul><li><code>_cmp_</code> is planned to be removed. Use <code>_richcmp_</code> instead.
</li></ul></blockquote>
<p>
Are you talking about <code>SkewPolynomial._cmp_</code>? We spent many hours debugging the comparison code because it threw strange errors and segfaults. To avoid me spending many more hours, could I ask you to tell us exactly how to write the correct comparison code then?
</p>
<p>
Simon King's tutorial on implementing algebraic structures should be updated to be more informative on this, I think.
</p>
<blockquote class="citation">
<ul><li>You should lazy import the ring constructor.
</li></ul></blockquote>
<p>
Do you mean <code>sage.rings.ring</code> in <code>skew_polynomial_ring.py</code>? Why is exactly that one important to lazily import?
</p>
<p>
Thanks for the help!
</p>
<p>
Best,
Johan
</p>
TicketarpitdmThu, 21 Jul 2016 20:33:11 GMT
https://trac.sagemath.org/ticket/13215#comment:77
https://trac.sagemath.org/ticket/13215#comment:77
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:75" title="Comment 75">tscrim</a>:
To add to what Johan is asking,
</p>
<blockquote class="citation">
<ul><li>Kill with fire and holy water <code>is_*</code> methods. They just trivially call <code>isinstance</code>.
</li></ul></blockquote>
<p>
Which "is_" methods are you referring to? I couldn't really find any that call isinstance directly apart from the is_SkewPolynomialRing. For example, <code></code>is_finite<code></code> returns true if the base ring is finite and has order 1 and false otherwise. And so on. Each have minor conditions that the user could of course check themselves if need be, but its not always a one line call and its not always obvious what methods are available to the user so that they may verify. Should such methods then still be removed?
</p>
<blockquote class="citation">
<ul><li>You are double importing things in the element class; only keep the <code>cimport</code>'s.
</li></ul></blockquote>
<p>
Could you elaborate with a tiny example or something what you mean by this?
</p>
<p>
Again, thank you for your help!
</p>
<p>
Best,
Arpit.
</p>
TickettscrimThu, 21 Jul 2016 21:41:24 GMT
https://trac.sagemath.org/ticket/13215#comment:78
https://trac.sagemath.org/ticket/13215#comment:78
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:77" title="Comment 77">arpitdm</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:75" title="Comment 75">tscrim</a>:
To add to what Johan is asking,
</p>
<blockquote class="citation">
<ul><li>Kill with fire and holy water <code>is_*</code> methods. They just trivially call <code>isinstance</code>.
</li></ul></blockquote>
<p>
Which "is_" methods are you referring to? I couldn't really find any that call isinstance directly apart from the is_SkewPolynomialRing. For example, <code></code>is_finite<code></code> returns true if the base ring is finite and has order 1 and false otherwise. And so on. Each have minor conditions that the user could of course check themselves if need be, but its not always a one line call and its not always obvious what methods are available to the user so that they may verify. Should such methods then still be removed?
</p>
</blockquote>
<p>
Sorry, I misspoke slightly. It is the <code>is_*</code> functions, which as you say are things like <code>is_SkewPolynomialRing</code>.
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>You are double importing things in the element class; only keep the <code>cimport</code>'s.
</li></ul></blockquote>
<p>
Could you elaborate with a tiny example or something what you mean by this?
</p>
</blockquote>
<p>
You should make this change:
</p>
<div class="wiki-code"><div class="code"><pre><span class="gd">-from sage.structure.element import Element, AlgebraElement
</span> from sage.structure.element cimport Element, AlgebraElement, ModuleElement
from sage.structure.parent cimport Parent
<span class="gd">-from polynomial_compiled import CompiledPolynomialFunction
</span> from polynomial_compiled cimport CompiledPolynomialFunction
</pre></div></div>
TickettscrimThu, 21 Jul 2016 22:00:10 GMT
https://trac.sagemath.org/ticket/13215#comment:79
https://trac.sagemath.org/ticket/13215#comment:79
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:76" title="Comment 76">jsrn</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:75" title="Comment 75">tscrim</a>:
</p>
<blockquote class="citation">
<ul><li>You seem to be using the exact same format of polynomials, which contains a lot of old code. For example, don't implement a <code>_repr_()</code> that just calls (a <em>Python</em> function) <code>_repr()</code>. This is useless (even in the usual polynomial ring code it seems); instead just implement <code>_repr_()</code>. Similarly, there are a number of methods you could simply <code>cpdef</code>.
</li></ul></blockquote>
<p>
That would be great. Can you give us the full list of the methods for which we can do this, or point us to how we can figure it out?
</p>
</blockquote>
<p>
Anything python method (a <code>def</code>) which only calls a <code>cdef</code> method. Another example is <code>list</code> calling <code>_list_c</code>. IIRC, you can't do this for <code>__hash__</code> because this is a Python special method. I think <code>_list_c</code> is the only other one (and you can replace this by just calling the attribute <code>__coeffs</code>).
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>You can use
<pre class="wiki">self._no_generic_basering_coercion = True
</pre>to avoid constructing the coercion from the base ring when initializing the skew polynomial ring.
</li></ul></blockquote>
<p>
To be precise, do you mean that can insert the above line in <code>SkewPolynomialRing_general.__init__</code> just before the call to <code>Algebra.__init__</code>, and then remove the call to <code>self._unset_coercions_used()</code>? Or is there more to it?
</p>
</blockquote>
<p>
Yes, exactly.
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li><code>_cmp_</code> is planned to be removed. Use <code>_richcmp_</code> instead.
</li></ul></blockquote>
<p>
Are you talking about <code>SkewPolynomial._cmp_</code>? We spent many hours debugging the comparison code because it threw strange errors and segfaults. To avoid me spending many more hours, could I ask you to tell us exactly how to write the correct comparison code then?
</p>
</blockquote>
<p>
You can look at <code>src/sage/rings/real_arb.pyx</code> or <code>src/sage/rings/real_double.pyx</code> for example. We can include the <code>_cmp_</code> as it is now, but it means some more hardship when we finally start converting.
</p>
<blockquote class="citation">
<p>
Simon King's tutorial on implementing algebraic structures should be updated to be more informative on this, I think.
</p>
</blockquote>
<p>
Yes, it should be. It was written before we really had to worry about going to Python3.
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>You should lazy import the ring constructor.
</li></ul></blockquote>
<p>
Do you mean <code>sage.rings.ring</code> in <code>skew_polynomial_ring.py</code>? Why is exactly that one important to lazily import?
</p>
</blockquote>
<p>
It is the thing which imports the rest of the code (once the <code>is_*</code> functions are gone). So all of the other files are not imported on startup, which helps keep that down because this is not a "common" object that the average user will use (IMO).
</p>
<blockquote class="citation">
<p>
Thanks for the help!
</p>
</blockquote>
<p>
Thank you for the good discussions and work.
</p>
TicketgitFri, 22 Jul 2016 22:48:25 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:80
https://trac.sagemath.org/ticket/13215#comment:80
<ul>
<li><strong>commit</strong>
changed from <em>e189fec13d005a7fba39a429876c501fe95c05da</em> to <em>22eab5df8c479f46d780c09625d3c7b790b3c816</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=1f62ef479a43b2402d4cf824c8b476aea40b9402"><span class="icon"></span>1f62ef4</a></td><td><code>added a section related to skew polynomils in the index file.</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=abbd05ee2ed097aac032318d79001134f87fa9e7"><span class="icon"></span>abbd05e</a></td><td><code>removed double imports and unused classes and methods</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=8b337c5eb2e3f3d88864660487ce93db06fba517"><span class="icon"></span>8b337c5</a></td><td><code>improved description of module, definition of skew polynomial, removed unnecessary imports, improved informativeness of docstrings, input sanitization and documentation of a couple of methods.</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=e0f3f421848b9a594a09e431d4eb0ac58e2b1d46"><span class="icon"></span>e0f3f42</a></td><td><code>changes to incorporate merging and into</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=0c8f6ec5fdae4746ce5b2989b426f78733ad3385"><span class="icon"></span>0c8f6ec</a></td><td><code>removed unnecessary imports</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=0641ecf3c9652b5479b967874a600cfd8a81c967"><span class="icon"></span>0641ecf</a></td><td><code>improved description of module, definition of skew polynomial ring, removed unnecessary imports, improved informativeness of docstrings, input sanitization and documentation of some methods. cleaned up code.</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=22eab5df8c479f46d780c09625d3c7b790b3c816"><span class="icon"></span>22eab5d</a></td><td><code>same as previous commit</code>
</td></tr></table>
TicketarpitdmFri, 22 Jul 2016 23:05:50 GMT
https://trac.sagemath.org/ticket/13215#comment:81
https://trac.sagemath.org/ticket/13215#comment:81
<p>
Stuff fixed in the commits above based on latest set of discussions:
</p>
<ol><li>Improved descriptions for element, ring and ring constructor files
</li><li>Improved definitions.
</li><li>Input sanitization and INPUT block for init methods
</li><li><code>name</code> and <code>names</code> merged into a single <code>names</code> argument.
</li><li>Improved documentation and tests for <code>twist_map</code>, make_generic_skew_polynomial` and other such methods.
</li><li>Removed all functions that trivially called <code>isinstance</code>. Merged _repr_ and _repr into _repr_.
</li><li>Improved call to coercion, removed double imports, removed unnecessary imports. Added lazy import where possible.
</li><li>Solving any doctests that came up in the course of the aforementioned changes.
</li></ol><p>
Please let me know if I have missed something and of course any other changes that ought to be made. I'm now opening this for review.
</p>
TicketarpitdmFri, 22 Jul 2016 23:06:06 GMTstatus changed
https://trac.sagemath.org/ticket/13215#comment:82
https://trac.sagemath.org/ticket/13215#comment:82
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
TicketjsrnSat, 23 Jul 2016 00:40:48 GMT
https://trac.sagemath.org/ticket/13215#comment:83
https://trac.sagemath.org/ticket/13215#comment:83
<p>
Sounds great!
</p>
<p>
Did you look at the <code>_cmp_</code> thing that Travis brought up?
Best,
Johan
</p>
TicketdlucasMon, 25 Jul 2016 09:06:24 GMTstatus changed
https://trac.sagemath.org/ticket/13215#comment:84
https://trac.sagemath.org/ticket/13215#comment:84
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Hello,
</p>
<p>
Good work on docstrings and module documentation, it's much better now.
However, I have some comments:
</p>
<ul><li>documentation does not compile: it seems you have some formatting errors in bullet lists.
</li><li>you used single backquote notation at some places in the documentation where it should be
double backquotes, e.g. in <code>SkewPolynomial</code> class, you wrote <code>self</code> between single backquotes (which triggers LaTeX formatting) instead of double.
</li><li>a few lines below, <code>default: False</code>, <code>False</code> should be between double backquotes.
</li><li>you kept the import <code>import sage.misc.latex as latex</code>. I'm quite sure you can remove it, but in that case, you'll have to change <code>latex.latex(self.base_ring())</code> to <code>self.base_ring()._latex_()</code> (l. 359, skew_polynomial_ring.py). There's probably some more to change.
</li></ul><p>
Apart from that, it looks good to me.
Patchbot says some doctests fails, but according to which doctests are failing, I'm quite sure it's not because of this ticket.
</p>
<p>
Best,
</p>
<p>
David
</p>
TicketgitMon, 25 Jul 2016 13:13:03 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:85
https://trac.sagemath.org/ticket/13215#comment:85
<ul>
<li><strong>commit</strong>
changed from <em>22eab5df8c479f46d780c09625d3c7b790b3c816</em> to <em>dc5fb565e1a2b126a86034cb1d1c5936c41b3d19</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=1c717db156e1f479c1bc6584c59fe0ae06f8c4b7"><span class="icon"></span>1c717db</a></td><td><code>resolved single and double back tick formatting inconsistencies.</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=6f4d80a54eb41d687a56dd34177f60da43777651"><span class="icon"></span>6f4d80a</a></td><td><code>resolved back tick inconsistencies from other files</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=dc5fb565e1a2b126a86034cb1d1c5936c41b3d19"><span class="icon"></span>dc5fb56</a></td><td><code>replaced method _cmp_ with _richcmp_. removed latex import.</code>
</td></tr></table>
TicketarpitdmMon, 25 Jul 2016 13:28:21 GMT
https://trac.sagemath.org/ticket/13215#comment:86
https://trac.sagemath.org/ticket/13215#comment:86
<p>
I think I managed to replace the _cmp_ method with _richcmp_. Please let me know if I did it incorrectly or something. Back tick inconsistencies are resolved. The latex import in skew_polynomial_element.pyx was removable, but the skew_polynomial_ring did not allow it. It threw up errors and I think its because the latex method of the self._map does not support it.
</p>
<p>
Also, I don't understand what is going wrong when I try to build the documentation. I can't find any indentation errors.
</p>
<pre class="wiki">[polynomia] loading pickled environment... not yet created
[polynomia] WARNING: intersphinx inventory '/home/arpit/Documents/GSOC_16/sage/local/share/doc/sage/inventory/en/reference/objects.inv' not fetchable due to <type 'exceptions.IOError'>: [Errno 2] No such file or directory: '/home/arpit/Documents/GSOC_16/sage/local/share/doc/sage/inventory/en/reference/objects.inv'
[polynomia] docstring of sage.rings.polynomial.skew_polynomial_element:16: WARNING: Literal block expected; none found.
[polynomia] docstring of sage.rings.polynomial.skew_polynomial_element:20: WARNING: Definition list ends without a blank line; unexpected unindent.
[polynomia] docstring of sage.rings.polynomial.skew_polynomial_element:30: WARNING: Literal block expected; none found.
[polynomia] docstring of sage.rings.polynomial.skew_polynomial_element.SkewPolynomial.left_gcd:11: WARNING: Bullet list ends without a blank line; unexpected unindent.
[polynomia] docstring of sage.rings.polynomial.skew_polynomial_element.SkewPolynomial.left_lcm:11: WARNING: Bullet list ends without a blank line; unexpected unindent.
[polynomia] docstring of sage.rings.polynomial.skew_polynomial_element.SkewPolynomial.left_xgcd:11: WARNING: Bullet list ends without a blank line; unexpected unindent.
[polynomia] docstring of sage.rings.polynomial.skew_polynomial_element.SkewPolynomial.right_gcd:11: WARNING: Bullet list ends without a blank line; unexpected unindent.
[polynomia] docstring of sage.rings.polynomial.skew_polynomial_element.SkewPolynomial.right_lcm:11: WARNING: Bullet list ends without a blank line; unexpected unindent.
[polynomia] docstring of sage.rings.polynomial.skew_polynomial_element.SkewPolynomial.right_xgcd:11: WARNING: Bullet list ends without a blank line; unexpected unindent.
[polynomia] docstring of sage.rings.polynomial.skew_polynomial_element.make_generic_skew_polynomial:12: WARNING: Literal block expected; none found.
[polynomia] /home/arpit/Documents/GSOC_16/sage/local/lib/python2.7/site-packages/sage/rings/polynomial/skew_polynomial_ring_constructor.py:docstring of sage.rings.polynomial.skew_polynomial_ring_constructor.SkewPolynomialRing:70: WARNING: Literal block expected; none found.
[polynomia] /home/arpit/Documents/GSOC_16/sage/local/lib/python2.7/site-packages/sage/rings/polynomial/skew_polynomial_ring_constructor.py:docstring of sage.rings.polynomial.skew_polynomial_ring_constructor.SkewPolynomialRing:105: WARNING: Literal block expected; none found.
[polynomia] /home/arpit/Documents/GSOC_16/sage/local/lib/python2.7/site-packages/sage/rings/polynomial/multi_polynomial_sequence.py:docstring of sage.rings.polynomial.multi_polynomial_sequence:3: WARNING: citation not found: CMR05
[polynomia] Exception occurred:
[polynomia] File "/home/arpit/Documents/GSOC_16/sage/src/sage_setup/docbuild/ext/multidocs.py", line 286, in link_static_files
[polynomia] os.symlink(master_static_dir, static_dir)
[polynomia] OSError: [Errno 17] File exists
[polynomia] The full traceback has been saved in /tmp/sphinx-err-IsQg2Y.log, if you want to report the issue to the developers.
[polynomia] Please also report this if it was a user error, so that a better error message can be provided next time.
[polynomia] A bug report can be filed in the tracker at <https://github.com/sphinx-doc/sphinx/issues>. Thanks!
Error building the documentation.
Traceback (most recent call last):
File "/home/arpit/Documents/GSOC_16/sage/local/lib/python/runpy.py", line 162, in _run_module_as_main
"__main__", fname, loader, pkg_name)
File "/home/arpit/Documents/GSOC_16/sage/local/lib/python/runpy.py", line 72, in _run_code
exec code in run_globals
File "/home/arpit/Documents/GSOC_16/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__main__.py", line 2, in <module>
main()
File "/home/arpit/Documents/GSOC_16/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 1629, in main
builder()
File "/home/arpit/Documents/GSOC_16/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 710, in _wrapper
getattr(DocBuilder, build_type)(self, *args, **kwds)
File "/home/arpit/Documents/GSOC_16/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 103, in f
runsphinx()
File "/home/arpit/Documents/GSOC_16/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/sphinxbuild.py", line 215, in runsphinx
raise exception
OSError: [polynomia] WARNING: intersphinx inventory '/home/arpit/Documents/GSOC_16/sage/local/share/doc/sage/inventory/en/reference/objects.inv' not fetchable due to <type 'exceptions.IOError'>: [Errno 2] No such file or directory: '/home/arpit/Documents/GSOC_16/sage/local/share/doc/sage/inventory/en/reference/objects.inv'
Note: incremental documentation builds sometimes cause spurious
error messages. To be certain that these are real errors, run
"make doc-clean" first and try again.
</pre><p>
</p>
TicketdlucasTue, 26 Jul 2016 08:00:34 GMT
https://trac.sagemath.org/ticket/13215#comment:87
https://trac.sagemath.org/ticket/13215#comment:87
<p>
Well, I don't know what happens...
Here's what I have on my local machine:
</p>
<pre class="wiki">[polynomia] docstring of sage.rings.polynomial.skew_polynomial_element:16: WARNING: Literal block expected; none found.
[polynomia] docstring of sage.rings.polynomial.skew_polynomial_element:20: WARNING: Definition list ends without a blank line; unexpected unindent.
[polynomia] docstring of sage.rings.polynomial.skew_polynomial_element:30: WARNING: Literal block expected; none found.
[polynomia] docstring of sage.rings.polynomial.skew_polynomial_element.SkewPolynomial.left_gcd:11: WARNING: Bullet list ends without a blank line; unexpected unindent.
[polynomia] docstring of sage.rings.polynomial.skew_polynomial_element.SkewPolynomial.left_lcm:11: WARNING: Bullet list ends without a blank line; unexpected unindent.
[polynomia] docstring of sage.rings.polynomial.skew_polynomial_element.SkewPolynomial.left_xgcd:11: WARNING: Bullet list ends without a blank line; unexpected unindent.
[polynomia] docstring of sage.rings.polynomial.skew_polynomial_element.SkewPolynomial.right_gcd:11: WARNING: Bullet list ends without a blank line; unexpected unindent.
[polynomia] docstring of sage.rings.polynomial.skew_polynomial_element.SkewPolynomial.right_lcm:11: WARNING: Bullet list ends without a blank line; unexpected unindent.
[polynomia] docstring of sage.rings.polynomial.skew_polynomial_element.SkewPolynomial.right_xgcd:11: WARNING: Bullet list ends without a blank line; unexpected unindent.
[polynomia] docstring of sage.rings.polynomial.skew_polynomial_element.make_generic_skew_polynomial:12: WARNING: Literal block expected; none found.
[polynomia] /home/david/Desktop/sage/local/lib/python2.7/site-packages/sage/rings/polynomial/skew_polynomial_ring_constructor.py:docstring of sage.rings.polynomial.skew_polynomial_ring_constructor.SkewPolynomialRing:73: WARNING: Literal block expected; none found.
[polynomia] /home/david/Desktop/sage/local/lib/python2.7/site-packages/sage/rings/polynomial/skew_polynomial_ring_constructor.py:docstring of sage.rings.polynomial.skew_polynomial_ring_constructor.SkewPolynomialRing:108: WARNING: Literal block expected; none found.
Error building the documentation.
Traceback (most recent call last):
File "/home/david/Desktop/sage/local/lib/python/runpy.py", line 162, in _run_module_as_main
"__main__", fname, loader, pkg_name)
File "/home/david/Desktop/sage/local/lib/python/runpy.py", line 72, in _run_code
exec code in run_globals
File "/home/david/Desktop/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__main__.py", line 2, in <module>
main()
File "/home/david/Desktop/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 1630, in main
builder()
File "/home/david/Desktop/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 710, in _wrapper
getattr(DocBuilder, build_type)(self, *args, **kwds)
File "/home/david/Desktop/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 103, in f
runsphinx()
File "/home/david/Desktop/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/sphinxbuild.py", line 215, in runsphinx
raise exception
OSError: [polynomia] docstring of sage.rings.polynomial.skew_polynomial_element:16: WARNING: Literal block expected; none found.
</pre><p>
This is the kind of error sphinx returns when there's no blank line after a bullet list.
Usually, it's because you did not indented properly if you put several lines in a single bullet.
It should be like this:
</p>
<pre class="wiki">- my first line
and my second line
</pre><p>
so it's not 4 whitespaces, it has to be aligned with the first character of the line above.
</p>
<p>
Anyway, for your issues, I'd say run <code>make doc-clean</code> and <code>make</code>.
</p>
<p>
David
</p>
TicketgitTue, 26 Jul 2016 13:47:45 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:88
https://trac.sagemath.org/ticket/13215#comment:88
<ul>
<li><strong>commit</strong>
changed from <em>dc5fb565e1a2b126a86034cb1d1c5936c41b3d19</em> to <em>0de3552ca85ee36a91cb41bff993e0eaebe25ecb</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=54099ac4744be60612d1afe7d25de01aeff5a881"><span class="icon"></span>54099ac</a></td><td><code>fixed errors that came up while building documentation</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=0de3552ca85ee36a91cb41bff993e0eaebe25ecb"><span class="icon"></span>0de3552</a></td><td><code>reference for CMR05 was missing from the docs in multi_polynomial_sequence.py file. Added that.</code>
</td></tr></table>
TicketarpitdmTue, 26 Jul 2016 13:57:27 GMT
https://trac.sagemath.org/ticket/13215#comment:89
https://trac.sagemath.org/ticket/13215#comment:89
<p>
Hi David,
</p>
<p>
I finally managed to make sense of the error messages that were showing up in the documentation. For instance,
[polynomia] docstring of sage.rings.polynomial.skew_polynomial_element:16: WARNING: Literal block expected; none found.
</p>
<p>
Represents some error in the 16th line of the documentation of the file and so on..
</p>
<p>
And like you suggested, one set of errors was because it indented incorrectly. Anyway, I think I've solved all the issues (docs build successfully on my system) and I do get a path to all the html files. Can you please confirm this?
</p>
TicketdlucasTue, 26 Jul 2016 14:54:49 GMT
https://trac.sagemath.org/ticket/13215#comment:90
https://trac.sagemath.org/ticket/13215#comment:90
<p>
Hello,
</p>
<blockquote class="citation">
<p>
I finally managed to make sense of the error messages that were showing up in the documentation. For instance, [polynomia] docstring of sage.rings.polynomial.skew_polynomial_element:16: WARNING: Literal block expected; none found.
</p>
</blockquote>
<p>
Oh, sorry, I thought you knew how to read these! Otherwise, I would have told you... It's not the most intuitive error message for sure, and I had some troubles with Sphinx compiler messages when I started working on Sage <code>:)</code>.
</p>
<p>
I just pulled your branch and I'm recompiling the reference manual.
If everything goes smoothly, I don't have other remarks, so if Johan and Travis agree, we can set this to <code>positive_review</code>.
</p>
TicketdlucasTue, 26 Jul 2016 15:09:25 GMTstatus, reviewer changed
https://trac.sagemath.org/ticket/13215#comment:91
https://trac.sagemath.org/ticket/13215#comment:91
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>reviewer</strong>
changed from <em>Burcin Erocal</em> to <em>Burcin Erocal, David Lucas</em>
</li>
</ul>
<p>
And it worked just fine.
So, documentation compiles, tests passes, you fixed what we asked you to, I'm fine with it.
If no one objects, I'll set this to <code>positive_review</code>.
</p>
<p>
David
</p>
TickettscrimTue, 26 Jul 2016 17:20:42 GMTstatus changed
https://trac.sagemath.org/ticket/13215#comment:92
https://trac.sagemath.org/ticket/13215#comment:92
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
The important thing is the first comment. The rest is more polish, but I think a fair amount more is needed.
</p>
<ul><li><em>All</em> methods and functions must have doctests (e.g., the one added to <code>map.pyx</code> and hidden methods). Also, I think you should remove the commented out code.
</li><li>You should remove this line from <code>module_list.py</code>:
<pre class="wiki"># depends = numpy_depends),
</pre></li><li>I would swap these lines in <code>rings.py</code>:
<div class="wiki-code"><div class="code"><pre>
<span class="gd">- from sage.categories.morphism import Morphism
</span> if isinstance(arg, tuple):
<span class="gi">+ from sage.categories.morphism import Morphism
</span> 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])
</pre></div></div></li><li>Remove all trailing whitespace (well, at least do not add any).
</li><li>In <code>skew_polynomial_element.pyx</code>, there are a number of things that need to be in latex mode (and an equation using the <code>.. MATH::</code> block).
</li><li>I would move most-to-all of the module-level doc to the user entry point and then have a <code>.. SEEALSO:: block referencing said doc</code>, so it is visible from the <code>?</code> on the command line. This also applies to the parent class(es?) (i.e. module-level to class-level).
</li><li>You should make <code>list</code> into a <code>cpdef</code> with an output type of <code>list</code> and remove <code>_list_c</code>.
</li><li>For <code>__iter__</code>, just iterate over <code>self.__coeffs</code>. Actually, a similar statement applies to many other places you call <code>_list_c</code> where you don't expose it to the user or modify the list (e.g., <code>__getitem__</code> and <code>_richcmp_</code>. The multivariate and sparse versions should be overriding these methods anyways.
</li><li>You should use the cached version of <code>0</code> instead of going through coercion and getting a new object in memory:
<div class="wiki-code"><div class="code"><pre> try:
l = (<SkewPolynomial>self)._list_c()[n]
return l
except IndexError:
<span class="gd">- c = self.base_ring()(0)
- return c
</span><span class="gi">+ return self.base_ring().zero()
</span></pre></div></div></li><li>Use Python3 syntax for errors: <code>raise IndexError("skew polynomials are immutable")</code>.
</li><li>Similar to above: <code>self.parent()(0)</code> -> <code>self.parent().zero()</code>.
</li><li><code>is_nilpotent</code> should have a short description of what the method should do (as well as the intended operation).
</li><li><code>Returns</code> -> <code>Return</code> in a few first-line of docstrings.
</li><li><code>.. Note::</code> -> <code>.. NOTE::</code> (this is a consistency thing).
</li><li>You have a blank <code>.. SEEALSO::</code> in <code>__floordiv__</code>.
</li><li><code>is_left_divisible_by</code> doesn't have a short description. Similar for other such methods.
</li><li>Error messages should start with a lowercase letter (this was something we've agreed upon in somewhat more recent history).
</li><li>There is a preference to have <code>``True``</code> in code formatting since it is a Python object/concept.
</li><li>Actually, a number of functions that use <code>_list_c</code> should be moved to the concrete class (and maybe an <code>@abstract_method</code> placeholder done in the ABC.
</li></ul><p>
I might have some more comments later too. However this is looking very good and it will be a great addition to Sage.
</p>
<p>
Also, the <code>_richcmp_</code> looks correct (although I don't have time right now to run any serious tests).
</p>
TicketjsrnTue, 26 Jul 2016 17:25:28 GMTreviewer, author changed
https://trac.sagemath.org/ticket/13215#comment:93
https://trac.sagemath.org/ticket/13215#comment:93
<ul>
<li><strong>reviewer</strong>
changed from <em>Burcin Erocal, David Lucas</em> to <em>Burcin Erocal, David Lucas, Travis Scrimshaw</em>
</li>
<li><strong>author</strong>
changed from <em>Xavier Caruso</em> to <em>Xavier Caruso, Arpit Merchant, Johan Rosenkilde</em>
</li>
</ul>
TicketjsrnWed, 27 Jul 2016 13:57:04 GMT
https://trac.sagemath.org/ticket/13215#comment:94
https://trac.sagemath.org/ticket/13215#comment:94
<p>
The implemented operator evaluation <code>__call__</code> is incorrect. For instance, <code>1(a) = a</code> and <code>x(a) = sigma(a)</code> in a correct implementation, but that is not the case here. The following implementation works:
</p>
<pre class="wiki"> if eval_pt not in self._parent:
raise TypeError("Evaluation point must be a ring element")
sigma = self._parent.twist_map()
coefficients = self.coefficients(sparse=False)
sum = self._parent.zero()
a = eval_pt
for c in coefficients:
sum += c * a
a = sigma(a)
return sum
</pre><p>
I think we should make an effort to cythonize this method. Could you try that Arpit?
</p>
<p>
Best,
Johan
</p>
TicketgitWed, 27 Jul 2016 15:21:10 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:95
https://trac.sagemath.org/ticket/13215#comment:95
<ul>
<li><strong>commit</strong>
changed from <em>0de3552ca85ee36a91cb41bff993e0eaebe25ecb</em> to <em>6fb186ab4b9eedb41596053854996263e85c5fe7</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=35364a9bc15d6af148c20933345c448326e86c07"><span class="icon"></span>35364a9</a></td><td><code>removed commented code. added documentation and test for :meth: inverse.</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=cede0966b41faeae67a458d59bb173a005c7e353"><span class="icon"></span>cede096</a></td><td><code>removed commented line of code.</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=17450fa64d2bfbd49a3ae9e37d721b3a4dabc412"><span class="icon"></span>17450fa</a></td><td><code>import of Morphism moved to inside of statement so that it acts like a lazy import.</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=5a369536d5b8bcdf5ad5bf82232bd89df71cc29a"><span class="icon"></span>5a36953</a></td><td><code>removed all trailing whitespaces. removed commented code from skew_polynomial_element.pxd.</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=bd4535103925cd697990e9c4e3068c33e86b7f52"><span class="icon"></span>bd45351</a></td><td><code>converted to Python3 syntax for errors. edited error messages to start with lower case letters.</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=c203856028bbdc4c45d90d84f63046231087f923"><span class="icon"></span>c203856</a></td><td><code>code formatting for True, False and None Python objects. added latex typsetting in docs. added descriptions for some methods that didn't have them.</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=b52fc9528249625e9498ac43f48376d4235f8b9d"><span class="icon"></span>b52fc95</a></td><td><code>using cached versions of 0 and 1 wherever applicable.</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=6fb186ab4b9eedb41596053854996263e85c5fe7"><span class="icon"></span>6fb186a</a></td><td><code>changed Returns to Return and Note to NOTE for consistency. Removed trailing whitespaces. codeformatting for True, False and None objects.</code>
</td></tr></table>
TicketarpitdmWed, 27 Jul 2016 15:31:35 GMT
https://trac.sagemath.org/ticket/13215#comment:96
https://trac.sagemath.org/ticket/13215#comment:96
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:92" title="Comment 92">tscrim</a>:
</p>
<blockquote class="citation">
<ul><li><em>All</em> methods and functions must have doctests (e.g., the one added to <code>map.pyx</code> and hidden methods). Also, I think you should remove the commented out code.
</li></ul></blockquote>
<p>
I am still going over the methods to check whether there are any remaining. And I still have some commented code to remove.
</p>
<blockquote class="citation">
<ul><li>I would move most-to-all of the module-level doc to the user entry point and then have a <code>.. SEEALSO:: block referencing said doc</code>, so it is visible from the <code>?</code> on the command line. This also applies to the parent class(es?) (i.e. module-level to class-level).
</li></ul></blockquote>
<p>
Could you please explain with an example maybe, what this means? Or maybe refer me to some method in some file from where I can read and understand.
</p>
<blockquote class="citation">
<ul><li>You should make <code>list</code> into a <code>cpdef</code> with an output type of <code>list</code> and remove <code>_list_c</code>.
</li></ul></blockquote>
<p>
class <code>SkewPolynomial_general</code> inherits from class <code>SkewPolynomial</code>. The <code>_list_c</code> method is implemented in the former while in the latter it is not. Are you suggesting that I remove _list_c from both classes and only have a <code>cpdef list list(self)</code> in the latter class?
</p>
<blockquote class="citation">
<ul><li>For <code>__iter__</code>, just iterate over <code>self.__coeffs</code>. Actually, a similar statement applies to many other places you call <code>_list_c</code> where you don't expose it to the user or modify the list (e.g., <code>__getitem__</code> and <code>_richcmp_</code>. The multivariate and sparse versions should be overriding these methods anyways.
</li></ul></blockquote>
<p>
There is no <code>__coeffs</code> attribute in class <code>SkewPolynomial</code> and it is actually defined in class <code>SkewPolynomial_general</code>. Is it Python convention to access from one class, an attribute with double leading underscores of another class? If so, what is the correct way of doing that?
</p>
<blockquote class="citation">
<ul><li>Actually, a number of functions that use <code>_list_c</code> should be moved to the concrete class (and maybe an <code>@abstract_method</code> placeholder done in the ABC.
</li></ul></blockquote>
<p>
Could you please elaborate a little what you mean by this? I'm not sure I follow.
</p>
TicketarpitdmWed, 27 Jul 2016 15:33:17 GMT
https://trac.sagemath.org/ticket/13215#comment:97
https://trac.sagemath.org/ticket/13215#comment:97
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:94" title="Comment 94">jsrn</a>:
</p>
<blockquote class="citation">
<p>
The implemented operator evaluation <code>__call__</code> is incorrect. For instance, <code>1(a) = a</code> and <code>x(a) = sigma(a)</code> in a correct implementation, but that is not the case here. The following implementation works:
</p>
</blockquote>
<p>
Oh! I now recall having this discussion before. Thank you for pointing it out again.
</p>
<blockquote class="citation">
<p>
I think we should make an effort to cythonize this method. Could you try that Arpit?
</p>
</blockquote>
<p>
Yup. On it. :)
</p>
<p>
Best,
Arpit
</p>
TickettscrimWed, 27 Jul 2016 18:02:50 GMT
https://trac.sagemath.org/ticket/13215#comment:98
https://trac.sagemath.org/ticket/13215#comment:98
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:96" title="Comment 96">arpitdm</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:92" title="Comment 92">tscrim</a>:
</p>
<blockquote class="citation">
<ul><li>I would move most-to-all of the module-level doc to the user entry point and then have a <code>.. SEEALSO:: block referencing said doc</code>, so it is visible from the <code>?</code> on the command line. This also applies to the parent class(es?) (i.e. module-level to class-level).
</li></ul></blockquote>
<p>
Could you please explain with an example maybe, what this means? Or maybe refer me to some method in some file from where I can read and understand.
</p>
</blockquote>
<p>
I would make a change like this:
</p>
<div class="wiki-code"><div class="code"><pre> """
Top/Module-level
<span class="gd">-Description
</span> """
class Foo:
"""
Class-level.
<span class="gi">+
+ Description
</span> """
</pre></div></div><p>
That way when you do <code>Foo?</code>, you get <code>Description</code>. You also want some cross-linking so that someone using the online doc has links to follow to get to <code>Description</code>.
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>You should make <code>list</code> into a <code>cpdef</code> with an output type of <code>list</code> and remove <code>_list_c</code>.
</li></ul></blockquote>
<p>
class <code>SkewPolynomial_general</code> inherits from class <code>SkewPolynomial</code>. The <code>_list_c</code> method is implemented in the former while in the latter it is not. Are you suggesting that I remove _list_c from both classes and only have a <code>cpdef list list(self)</code> in the latter class?
</p>
<blockquote class="citation">
<ul><li>For <code>__iter__</code>, just iterate over <code>self.__coeffs</code>. Actually, a similar statement applies to many other places you call <code>_list_c</code> where you don't expose it to the user or modify the list (e.g., <code>__getitem__</code> and <code>_richcmp_</code>. The multivariate and sparse versions should be overriding these methods anyways.
</li></ul></blockquote>
<p>
There is no <code>__coeffs</code> attribute in class <code>SkewPolynomial</code> and it is actually defined in class <code>SkewPolynomial_general</code>. Is it Python convention to access from one class, an attribute with double leading underscores of another class? If so, what is the correct way of doing that?
</p>
</blockquote>
<p>
Don't make it double underscore (you can also do the explicit name-mangling). In general, I avoid double underscores so you can use the hidden attributes in subclasses (unless you really wanted it to be completely hidden from all subclasses, which is very rare).
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>Actually, a number of functions that use <code>_list_c</code> should be moved to the concrete class (and maybe an <code>@abstract_method</code> placeholder done in the ABC.
</li></ul></blockquote>
<p>
Could you please elaborate a little what you mean by this? I'm not sure I follow.
</p>
</blockquote>
<p>
ABC = Abstract Base Class, so the methods you have that just raise a <code>NotImplementedError</code> (e.g., <code>_inplace_add</code>), you should have them be blank (modulo a docstring and a doctest of a concrete implementation) with a <code>@abstract_method</code>. These then get picked up by a failure of the <code>TestSuite</code>.
</p>
TicketgitWed, 27 Jul 2016 22:38:13 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:99
https://trac.sagemath.org/ticket/13215#comment:99
<ul>
<li><strong>commit</strong>
changed from <em>6fb186ab4b9eedb41596053854996263e85c5fe7</em> to <em>271bface01f93c8dca5a1fb104da139224ae1e66</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=1204f2d240bedaa2c7eef1bcba144b73150d9308"><span class="icon"></span>1204f2d</a></td><td><code>the current implementation of the :meth: __call__ was incorrect. added the correct and cythonized version.</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=7e076fa503d41f940a4a363d92621369c128a0a5"><span class="icon"></span>7e076fa</a></td><td><code>moved module level docs to user entry point at the class level so that they are accessible at command line. added a SEEALSO block.</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=69fecc4c93cfb17f0e348069a95b44e53ac79236"><span class="icon"></span>69fecc4</a></td><td><code>converted :meth: into a cpdef method with output type list. removed and directly use the attribute.</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=271bface01f93c8dca5a1fb104da139224ae1e66"><span class="icon"></span>271bfac</a></td><td><code>removed some commented code and trailing whitespaces.</code>
</td></tr></table>
TicketarpitdmWed, 27 Jul 2016 22:46:10 GMT
https://trac.sagemath.org/ticket/13215#comment:100
https://trac.sagemath.org/ticket/13215#comment:100
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:98" title="Comment 98">tscrim</a>:
</p>
<blockquote class="citation">
<blockquote class="citation">
<blockquote class="citation">
<ul><li>Actually, a number of functions that use <code>_list_c</code> should be moved to the concrete class (and maybe an <code>@abstract_method</code> placeholder done in the ABC.
</li></ul></blockquote>
<p>
Could you please elaborate a little what you mean by this? I'm not sure I follow.
</p>
</blockquote>
<p>
ABC = Abstract Base Class, so the methods you have that just raise a <code>NotImplementedError</code> (e.g., <code>_inplace_add</code>), you should have them be blank (modulo a docstring and a doctest of a concrete implementation) with a <code>@abstract_method</code>. These then get picked up by a failure of the <code>TestSuite</code>.
</p>
</blockquote>
<p>
I tried adding the @abstract_method to the <code>cdef void _inplace_add</code> method and I got an error message saying "Cdef functions/classes cannot take arbitrary decorators." According to <a class="ext-link" href="http://docs.cython.org/en/latest/src/tutorial/pure.html"><span class="icon"></span>their docs</a>, I don't think decorators are implemented yet.
</p>
TicketarpitdmThu, 28 Jul 2016 10:31:13 GMTstatus changed
https://trac.sagemath.org/ticket/13215#comment:101
https://trac.sagemath.org/ticket/13215#comment:101
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
TickettscrimThu, 28 Jul 2016 16:31:41 GMT
https://trac.sagemath.org/ticket/13215#comment:102
https://trac.sagemath.org/ticket/13215#comment:102
<p>
Oh, I missed that they were <code>cdef</code> methods. I don't think you need to declare anything for them for ABCs, but I am not completely sure if Cython allows this.
</p>
TicketdlucasMon, 01 Aug 2016 09:21:14 GMTstatus changed
https://trac.sagemath.org/ticket/13215#comment:103
https://trac.sagemath.org/ticket/13215#comment:103
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Hello,
</p>
<p>
Some doctests are still missing. If you're not sure you documented everything, use the following command: <code>sage -coverage <path_to_files></code>. On my side, it returns:
</p>
<pre class="wiki">david@david:~/Desktop/sage/src/sage/rings/polynomial$ sage -coverage skew_polynomial_*
------------------------------------------------------------------------
SCORE skew_polynomial_element.pyx: 97.4% (76 of 78)
Missing doctests:
* line 327: cpdef SkewPolynomial _new_constant_poly(self, RingElement a, Parent P, char check=0)
* line 892: def is_nilpotent(self)
Possibly wrong (function name doesn't occur in doctests):
* line 2434: def mod(self, other)
* line 3129: cpdef Element _call_with_args(self, x, args=(), kwds={})
------------------------------------------------------------------------
SCORE skew_polynomial_ring_constructor.py: 100.0% (1 of 1)
------------------------------------------------------------------------
SCORE skew_polynomial_ring.py: 78.9% (15 of 19)
Missing documentation:
* line 129: def __classcall__(cls, base_ring, map, name=None, sparse=False, element_class=None)
Missing doctests:
* line 191: def __reduce__(self)
* line 514: def parameter(self)
* line 567: def is_sparse(self)
</pre><p>
Also, documentation does not compile:
</p>
<pre class="wiki">sage.rings.polynomial.skew_polynomial_ring.SkewPolynomialRing_general:53:
WARNING: Block quote ends without a blank line; unexpected unindent.
</pre><ul><li>line 872 in <code>skew_polynomial_element.pyx</code>, I think it would be better to return an error message with the exception <code>ValueError</code>.
</li></ul><ul><li>There are some undocumented methods/classes in <code>skew_polynomial_element.pyx</code> (starting at line 2776 with <code>_left_pow</code>).
</li></ul><ul><li>There's still one old style <code>raise</code> statement (l. 494 in <code>skew_polynomial_ring.py</code>).
</li></ul><p>
David
</p>
TicketjsrnMon, 01 Aug 2016 12:52:02 GMTbranch changed
https://trac.sagemath.org/ticket/13215#comment:104
https://trac.sagemath.org/ticket/13215#comment:104
<ul>
<li><strong>branch</strong>
changed from <em>u/arpitdm/skew_polynomials</em> to <em>u/jsrn/skew_polynomials</em>
</li>
</ul>
TicketjsrnMon, 01 Aug 2016 12:52:41 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:105
https://trac.sagemath.org/ticket/13215#comment:105
<ul>
<li><strong>commit</strong>
changed from <em>271bface01f93c8dca5a1fb104da139224ae1e66</em> to <em>4a33c5fd34940d28a3824971a4fe4710c04ab2b2</em>
</li>
</ul>
<p>
I fixed a minor issue with <code>src/module_list.py</code>.
</p>
<hr />
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=362fff7add8c4181e683a7eef61fa01b5f42d6e6"><span class="icon"></span>362fff7</a></td><td><code>Merge branch 'u/arpitdm/skew_polynomials' of git://trac.sagemath.org/sage into 13215_skew_polynomials</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=4a33c5fd34940d28a3824971a4fe4710c04ab2b2"><span class="icon"></span>4a33c5f</a></td><td><code>mv skew_polynomial_element in module_list</code>
</td></tr></table>
TicketjsrnMon, 01 Aug 2016 14:25:17 GMT
https://trac.sagemath.org/ticket/13215#comment:106
https://trac.sagemath.org/ticket/13215#comment:106
<ul><li>Is there any sense to the function <code>make_generic_skew_polynomial</code>? Shouldn't
it just be removed? (incidentally, I don't see the sense of
<code>sage.rings.polynomial.polynomial_element.make_generic_polynomial</code> either,
which presumably was the inspiration to <code>make_generic_skew_polynomial</code>.)
</li></ul><ul><li>You created <code>_leftpow_</code> and <code>_rightpow_</code> from <code>__pow__</code> to avoid the optional
argument <code>side</code>. But they are not standard magic functions, so there's no
sense in them being underscore-methods. Rather, they should be called
something different. E.g <code>power_left_mod</code> and <code>power_right_mod</code>, where
<code>modulus</code> is now a mandatory argument.
</li></ul><ul><li>I'm worried about the <code>_inplace_*</code> methods: they are not doc-tested but are
also not indirectly used anywhere (except <code>_inplace_pow</code> which is used in
<code>_leftpow</code> and <code>_inplace_rmul</code> which is used in <code>_inplace_pow</code>). I only tried
to read <code>_inplace_add</code> yet, but already found one bug (duplicate line `x +=
y[len(x):]`). I would prefer that these methods be removed, possibly
reinstated at a later time when they will be used (and hence at least
slightly doctested). I believe they were more arduously used in the Karatsuba
or factorisation implementations?
</li></ul><p>
Best,
Johan
</p>
TicketarpitdmMon, 01 Aug 2016 14:41:04 GMT
https://trac.sagemath.org/ticket/13215#comment:107
https://trac.sagemath.org/ticket/13215#comment:107
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:103" title="Comment 103">dlucas</a>:
</p>
<blockquote class="citation">
<ul><li>line 892: def is_nilpotent(self)
</li></ul></blockquote>
<p>
The <code>is_nilpotent</code> method is not implemented since order of the automorphism is not supported in Sage yet. How about if I remove this method completely and place the note instead in <code>def is_unit</code>? Or should I leave it as it is?
</p>
<blockquote class="citation">
<p>
Possibly wrong (function name doesn't occur in doctests):
</p>
<ul><li>line 3129: cpdef Element _call_with_args(self, x, args=(), kwds={})
</li></ul></blockquote>
<p>
There is a <code>cpdef Element _call_(self, x)</code> method that does exactly the same thing as the above method. Do we need to keep both or can we keep the more general <code>_call_with_args</code> and remove <code>_call_</code>?
</p>
TicketarpitdmMon, 01 Aug 2016 14:46:45 GMT
https://trac.sagemath.org/ticket/13215#comment:108
https://trac.sagemath.org/ticket/13215#comment:108
<blockquote class="citation">
<ul><li>Is there any sense to the function <code>make_generic_skew_polynomial</code>? Shouldn't
it just be removed? (incidentally, I don't see the sense of
<code>sage.rings.polynomial.polynomial_element.make_generic_polynomial</code> either,
which presumably was the inspiration to <code>make_generic_skew_polynomial</code>.)
</li></ul></blockquote>
<p>
Yup. I believe that was the inspiration. And I agree. If one wants to make a generic skew polynomial using coefficients, it can be directly done without using this method.
</p>
<blockquote class="citation">
<ul><li>I'm worried about the <code>_inplace_*</code> methods: they are not doc-tested but are
also not indirectly used anywhere (except <code>_inplace_pow</code> which is used in
<code>_leftpow</code> and <code>_inplace_rmul</code> which is used in <code>_inplace_pow</code>). I only tried
to read <code>_inplace_add</code> yet, but already found one bug (duplicate line `x +=
y[len(x):]`). I would prefer that these methods be removed, possibly
reinstated at a later time when they will be used (and hence at least
slightly doctested). I believe they were more arduously used in the Karatsuba
or factorisation implementations?
</li></ul></blockquote>
<p>
Yes. They are used in factorizations and center related methods. Which is why it was not possible to add indirect tests for them either. Alright, removing <code>_inplace_add</code>, <code>_inplace_sub</code> and <code>_inplace_rmul</code>.
</p>
TicketgitMon, 01 Aug 2016 14:52:58 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:109
https://trac.sagemath.org/ticket/13215#comment:109
<ul>
<li><strong>commit</strong>
changed from <em>4a33c5fd34940d28a3824971a4fe4710c04ab2b2</em> to <em>7134b7e0af8f1764aa8d8d71d2ef7f3a6582bd8f</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=a921040a9d63165a00b2b6d364335a0a6b155634"><span class="icon"></span>a921040</a></td><td><code>Changed unfortunate parameter name</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=f43767a9714b68fc568b9e1805edf6bd38fada45"><span class="icon"></span>f43767a</a></td><td><code>Some minor doc and error message modifications</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=7c5c51146d4c044c3bceb0b77abcbbbd77ee3c1b"><span class="icon"></span>7c5c511</a></td><td><code>Modelled SkewPolynomialBaseringInjection more closely on PolynomialBaseringInjection</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=d6fd90460cd27a56ea35a5c9917b418158233f86"><span class="icon"></span>d6fd904</a></td><td><code>Fix Cython warning</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=7134b7e0af8f1764aa8d8d71d2ef7f3a6582bd8f"><span class="icon"></span>7134b7e</a></td><td><code>Removed _call_with_args</code>
</td></tr></table>
TicketjsrnMon, 01 Aug 2016 14:57:20 GMT
https://trac.sagemath.org/ticket/13215#comment:110
https://trac.sagemath.org/ticket/13215#comment:110
<p>
I fixed some other stuff that I came across. Most importantly, I changed the parent type of <code>SkewPolynomialBaseringInjection</code> so that it mimics <code>PolynomialBaseringInjection</code>: I'm not fully understanding the extend of what this means, but if <code>PolynomialBaseringInjection</code> should be a <code>Morphism</code> and not a <code>RingHomomorphism</code>, then I don't see why it should be different for <code>SkewPolynomialBaseringInjection</code>.
</p>
<p>
I also removed <code>_call_with_args</code>: as the name says, it's there for supporting calling an object that takes more than one argument. This makes sense in the very general <code>PolynomialBaseringInjection</code> as the example in the source shows. But it doesn't (yet) make any sense for skew polynomial rings, so I've removed it for now. If it should ever be useful, someone can add it at that time. For now, it will simply throw a (somewhat meaningful) error when one tries to apply more than one argument to a call to a <code>SkewPolynomialBaseringInjection</code>.
</p>
TicketcarusoMon, 01 Aug 2016 22:41:13 GMT
https://trac.sagemath.org/ticket/13215#comment:111
https://trac.sagemath.org/ticket/13215#comment:111
<p>
Many many thanks (to all of you) for all your hard work on this ticket!
I really appreciate it and apologize for being silent for such a long time.
</p>
<p>
I'm looking forward to seeing you soon in <a class="missing wiki">SageDays?</a> 75.
</p>
TicketarpitdmTue, 02 Aug 2016 00:24:26 GMTbranch changed
https://trac.sagemath.org/ticket/13215#comment:112
https://trac.sagemath.org/ticket/13215#comment:112
<ul>
<li><strong>branch</strong>
changed from <em>u/jsrn/skew_polynomials</em> to <em>u/arpitdm/skew_polynomials</em>
</li>
</ul>
TicketarpitdmTue, 02 Aug 2016 00:25:18 GMTstatus, commit changed
https://trac.sagemath.org/ticket/13215#comment:113
https://trac.sagemath.org/ticket/13215#comment:113
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>commit</strong>
changed from <em>7134b7e0af8f1764aa8d8d71d2ef7f3a6582bd8f</em> to <em>dfb6b1db991b198e5616994625726e07a067b56f</em>
</li>
</ul>
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=d48fea6af3d41939ce1c88ed835b703125a17aa4"><span class="icon"></span>d48fea6</a></td><td><code>fixed documentation errors</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=fae04830a6aa99e52ac7ca50736d7df3a3281e33"><span class="icon"></span>fae0483</a></td><td><code>fixed documentation</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=dfb6b1db991b198e5616994625726e07a067b56f"><span class="icon"></span>dfb6b1d</a></td><td><code>removed unused methods. modified leftpow and rightpow methods.</code>
</td></tr></table>
TicketjsrnTue, 02 Aug 2016 07:34:08 GMT
https://trac.sagemath.org/ticket/13215#comment:114
https://trac.sagemath.org/ticket/13215#comment:114
<p>
There's a bug in <code>right_lcm</code>. I don't yet know what it is but here is an example which makes it blow up:
</p>
<pre class="wiki">k.<a> = GF(3^2,'a')
sigma = k.frobenius_endomorphism()
S.<x> = k['x', sigma]
(x^2 + 1*x + a).right_lcm(x - a)
<BOOM>
NotImplementedError: the leading coefficient is not a unit
</pre><p>
Since we're over a field, it must be the <code>ZeroDivisionError</code> causing this, which should never happen.
</p>
<p>
I also don't see why we should catch the <code>ZeroDivisionError</code>: it is never
expected behaviour that <code>V3</code> is 0?
</p>
<p>
As an aside, the naming in this and related methods is terrible: <code>R</code> means two very different things in 10 lines, and the skew polynomial variables should not be capitalised.
</p>
<p>
Best,
Johan
</p>
TicketcarusoTue, 02 Aug 2016 08:11:30 GMT
https://trac.sagemath.org/ticket/13215#comment:115
https://trac.sagemath.org/ticket/13215#comment:115
<p>
The problem comes from the multiplication by the skew polynomial 0 which does not work as expected (it returns a polynomial of positive degree):
</p>
<pre class="wiki">sage: k.<a> = GF(5^2)
sage: S.<x> = SkewPolynomialRing(k, k.frobenius_endomorphism())
sage: zero = S(0)*x
sage: zero.degree()
1
sage: zero + 1
+ 1
</pre>
TicketjsrnTue, 02 Aug 2016 09:43:53 GMTbranch changed
https://trac.sagemath.org/ticket/13215#comment:116
https://trac.sagemath.org/ticket/13215#comment:116
<ul>
<li><strong>branch</strong>
changed from <em>u/arpitdm/skew_polynomials</em> to <em>u/jsrn/skew_polynomials</em>
</li>
</ul>
TicketjsrnTue, 02 Aug 2016 09:44:48 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:117
https://trac.sagemath.org/ticket/13215#comment:117
<ul>
<li><strong>commit</strong>
changed from <em>dfb6b1db991b198e5616994625726e07a067b56f</em> to <em>ca15975841eaa6a05537b626297baf1bcda6d896</em>
</li>
</ul>
<p>
Thanks Xavier: it turned out that the problem was even with <code>S(0)</code>, which got represented as <code>[0]</code> instead of <code>[]</code>. I fixed it now and added a test for posterity.
</p>
<hr />
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=291c28c76171bb1c8a4787ae9b4681d687fb876a"><span class="icon"></span>291c28c</a></td><td><code>Fixed construct-0 bug</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=ca15975841eaa6a05537b626297baf1bcda6d896"><span class="icon"></span>ca15975</a></td><td><code>Fix doctest</code>
</td></tr></table>
TicketgitTue, 02 Aug 2016 11:13:25 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:118
https://trac.sagemath.org/ticket/13215#comment:118
<ul>
<li><strong>commit</strong>
changed from <em>ca15975841eaa6a05537b626297baf1bcda6d896</em> to <em>ba6a753d47d8158d0c14e129f91d15fb71a64b27</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=ba6a753d47d8158d0c14e129f91d15fb71a64b27"><span class="icon"></span>ba6a753</a></td><td><code>Fix mathematically incorrect doc-string</code>
</td></tr></table>
TicketjsrnTue, 02 Aug 2016 11:13:55 GMT
https://trac.sagemath.org/ticket/13215#comment:119
https://trac.sagemath.org/ticket/13215#comment:119
<p>
Small doc-string fix in the <code>lcm</code>-functions.
</p>
TicketjdemeyerWed, 03 Aug 2016 12:15:56 GMTstatus changed
https://trac.sagemath.org/ticket/13215#comment:120
https://trac.sagemath.org/ticket/13215#comment:120
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Just skimming the code, not a full review...
</p>
<ol><li>All this
<pre class="wiki"> if op == 0:
return x < y
elif op == 1:
return x <= y
elif op == 2:
return x == y
elif op == 3:
return x != y
elif op == 4:
return x > y
else:
return x >= y
</pre></li></ol><p>
can be replaced by <code>return PyObject_RichCompare(x, y, op)</code>, see <code>src/sage/structure/coerce.pyx</code> for an example.
</p>
<ol start="2"><li>Lots of private methods are unused:
<pre class="wiki"> cdef void _inplace_add(self, SkewPolynomial_generic_dense right)
cdef void _inplace_sub(self, SkewPolynomial_generic_dense right)
cdef void _inplace_lmul(self, SkewPolynomial_generic_dense right)
cpdef _leftpow_(self,right,modulus=*)
</pre></li></ol><p>
Since these are private, I see no point in implementing them.
</p>
<ol start="3"><li>The <code>.pxd</code> file contains too much stuff. In particular, the following all seems unused:
<pre class="wiki">include "cysignals/signals.pxi"
include "../../ext/cdefs.pxi"
include '../../ext/stdsage.pxi'
from sage.rings.integer cimport Integer
from polynomial_compiled cimport CompiledPolynomialFunction
</pre></li></ol><p>
Some of this might be needed in <code>.pyx</code> files, but then it should be in the <code>.pyx</code> file. The <code>include</code> statements for <code>cdefs.pxi</code> and <code>stdsage.pxi</code> should be replaced instead by the relevant <code>cimport</code> statements.
</p>
<ol start="4"><li>Use the standard copyright template from <a class="ext-link" href="http://doc.sagemath.org/html/en/developer/coding_basics.html#headings-of-sage-library-code-files"><span class="icon"></span>http://doc.sagemath.org/html/en/developer/coding_basics.html#headings-of-sage-library-code-files</a>
</li></ol><ol start="5"><li><code>__getslice__</code> is deprecated.
</li></ol><ol start="6"><li><code>__mod__</code> should use the coercion model (<a class="closed ticket" href="https://trac.sagemath.org/ticket/269" title="enhancement: Add __mod__ to coercion model (closed: fixed)">#269</a>).
</li></ol><ol start="7"><li><code>__floordiv__</code> should use the coercion model (<a class="closed ticket" href="https://trac.sagemath.org/ticket/2034" title="defect: __floordiv__ should be part of coercion model (closed: fixed)">#2034</a>).
</li></ol><ol start="8"><li>Replace <code>trac #13215</code> by <code>:trac:`13215`</code>
</li></ol><ol start="9"><li>In several places, you use exact type checks of the form <code>type(x) is foo</code>. Why not <code>isinstance(x, foo)</code>?
</li></ol>
TicketjdemeyerWed, 03 Aug 2016 12:18:02 GMT
https://trac.sagemath.org/ticket/13215#comment:121
https://trac.sagemath.org/ticket/13215#comment:121
<ol start="10"><li>according to the patchbot, the documentation does not build.
</li></ol>
TicketdlucasWed, 03 Aug 2016 13:40:50 GMT
https://trac.sagemath.org/ticket/13215#comment:122
https://trac.sagemath.org/ticket/13215#comment:122
<p>
Hello Jeroen,
</p>
<blockquote class="citation">
<ol start="10"><li>according to the patchbot, the documentation does not build.
</li></ol></blockquote>
<p>
The patchbot might have used an older version of this ticket, because documentation builds just fine on my side.
</p>
<p>
David
</p>
TicketarpitdmThu, 04 Aug 2016 06:57:11 GMTbranch changed
https://trac.sagemath.org/ticket/13215#comment:123
https://trac.sagemath.org/ticket/13215#comment:123
<ul>
<li><strong>branch</strong>
changed from <em>u/jsrn/skew_polynomials</em> to <em>u/arpitdm/skew_polynomials</em>
</li>
</ul>
TicketarpitdmThu, 04 Aug 2016 07:10:41 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:124
https://trac.sagemath.org/ticket/13215#comment:124
<ul>
<li><strong>commit</strong>
changed from <em>ba6a753d47d8158d0c14e129f91d15fb71a64b27</em> to <em>f563aba916b4155a9485c46ebbbdfe3404a2c8f1</em>
</li>
</ul>
<p>
Hi Jeroen,
</p>
<p>
I've made changes to the ticket based on your comments. Please do let me know if I've made a mistake and if there are any other changes I should be making.
</p>
<p>
Also, as David mentioned, the documentation builds on my system as well. I don't know the patchbot is suggesting otherwise.
</p>
<p>
Best,
Arpit.
</p>
<hr />
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=d48fea6af3d41939ce1c88ed835b703125a17aa4"><span class="icon"></span>d48fea6</a></td><td><code>fixed documentation errors</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=fae04830a6aa99e52ac7ca50736d7df3a3281e33"><span class="icon"></span>fae0483</a></td><td><code>fixed documentation</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=dfb6b1db991b198e5616994625726e07a067b56f"><span class="icon"></span>dfb6b1d</a></td><td><code>removed unused methods. modified leftpow and rightpow methods.</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=2ea89c11d37e8187b4c543b0d6ce83980866340a"><span class="icon"></span>2ea89c1</a></td><td><code>merged new updates from remote</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=5c9f2f3d194b396dae408c59f2b3d9ad9d8cf891"><span class="icon"></span>5c9f2f3</a></td><td><code>modified richcmp method. removed some inplace methods.</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=c034df844e697939c159552844b089e787bededd"><span class="icon"></span>c034df8</a></td><td><code>removed unused private methods and imports</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=ac109c82509d2ab9bcf7539cd1b7b86a5b1a563b"><span class="icon"></span>ac109c8</a></td><td><code>converted to standard copyright template</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=5137ef91cbdc7fe0d7712905da336d5a92716713"><span class="icon"></span>5137ef9</a></td><td><code>removed deprecated method getslice</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=b1e42c3f1b032ca62389dd68ded1fece79678955"><span class="icon"></span>b1e42c3</a></td><td><code>Merge commit 'aca3398a81b3688e1f2e69c5910b8214d13be925' of git://trac.sagemath.org/sage into skew_polynomials</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=f563aba916b4155a9485c46ebbbdfe3404a2c8f1"><span class="icon"></span>f563aba</a></td><td><code>mod and floordiv modified to use coercion model. replaced type with isinstance.</code>
</td></tr></table>
TicketjdemeyerThu, 04 Aug 2016 07:33:13 GMTdependencies deleted
https://trac.sagemath.org/ticket/13215#comment:125
https://trac.sagemath.org/ticket/13215#comment:125
<ul>
<li><strong>dependencies</strong>
<em>#13214, #13303, #13640, #13641, #13642</em> deleted
</li>
</ul>
TicketjdemeyerThu, 04 Aug 2016 07:37:46 GMT
https://trac.sagemath.org/ticket/13215#comment:126
https://trac.sagemath.org/ticket/13215#comment:126
<p>
Just a remark (feel free to change this or not): there is no point in writing <code>cdef list</code> here since you're not using the fact that it's a <code>list</code>:
</p>
<pre class="wiki"> cdef list x = (<SkewPolynomial>left)._coeffs
cdef list y = (<SkewPolynomial>right)._coeffs
return PyObject_RichCompare(x, y, op)
</pre><p>
In general one should avoid such pointless type declarations.
</p>
<p>
In this case, it doesn't hurt since Cython knows anyway that it's a list (<code>_coeffs</code> is declared to be a <code>list</code>).
</p>
TicketjdemeyerThu, 04 Aug 2016 07:43:25 GMT
https://trac.sagemath.org/ticket/13215#comment:127
https://trac.sagemath.org/ticket/13215#comment:127
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:124" title="Comment 124">arpitdm</a>:
</p>
<blockquote class="citation">
<p>
I've made changes to the ticket based on your comments. Please do let me know if I've made a mistake and if there are any other changes I should be making.
</p>
</blockquote>
<p>
You forgot to change the copyright statements. Apart from that, it looks ok.
</p>
TicketarpitdmThu, 04 Aug 2016 08:08:17 GMT
https://trac.sagemath.org/ticket/13215#comment:128
https://trac.sagemath.org/ticket/13215#comment:128
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:127" title="Comment 127">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:124" title="Comment 124">arpitdm</a>:
</p>
<blockquote class="citation">
<p>
I've made changes to the ticket based on your comments. Please do let me know if I've made a mistake and if there are any other changes I should be making.
</p>
</blockquote>
<p>
You forgot to change the copyright statements. Apart from that, it looks ok.
</p>
</blockquote>
<p>
By copyright, do you mean add names inside the block too? I added myself and Johan as authors (commit ac109c8)
</p>
<p>
As for the cdef list, I went with the rest of the code on the ticket which declares things similarly at the time. But I can change that.
</p>
TicketdlucasThu, 04 Aug 2016 08:16:48 GMT
https://trac.sagemath.org/ticket/13215#comment:129
https://trac.sagemath.org/ticket/13215#comment:129
<blockquote class="citation">
<p>
By copyright, do you mean add names inside the block too? I added myself and Johan as authors (commit ac109c8)
</p>
</blockquote>
<p>
Actually, there's a template for copyrights in Sage (Jeroen sent you the link yesterday).
</p>
<p>
The copyright statements in this ticket are like this:
</p>
<pre class="wiki">#############################################################################
# Copyright (C) 2012 Xavier Caruso <xavier.caruso@normalesup.org>
#
# Distributed under the terms of the GNU General Public License (GPL)
#
# http://www.gnu.org/licenses/
#****************************************************************************
</pre><p>
while they should be like this
</p>
<pre class="wiki">#*****************************************************************************
# Copyright (C) 2013 YOUR NAME <your email>
#
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation, either version 2 of the License, or
# (at your option) any later version.
# http://www.gnu.org/licenses/
#*****************************************************************************
</pre><p>
David
</p>
TicketjsrnThu, 04 Aug 2016 08:22:31 GMT
https://trac.sagemath.org/ticket/13215#comment:130
https://trac.sagemath.org/ticket/13215#comment:130
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:128" title="Comment 128">arpitdm</a>:
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
By copyright, do you mean add names inside the block too? I added myself and Johan as authors (commit ac109c8)
</p>
</blockquote>
</blockquote>
<p>
You misspelled my last name. But I've actually recently changed my name to "Johan Rosenkilde".
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:129" title="Comment 129">dlucas</a>:
</p>
<blockquote class="citation">
<p>
while they should be like this
</p>
<pre class="wiki">#*****************************************************************************
# Copyright (C) 2013 YOUR NAME <your email>
</pre></blockquote>
<p>
OK, stupid copyright stuff leading to an uncomfortable discussion: I think the copyright name should be retained as Xavier Caruso. Arpit should add his name in the list of AUTHORS as he has done (but he should of course fix the rest of the banner).
</p>
<p>
Best,
Johan
</p>
TicketdlucasThu, 04 Aug 2016 08:34:10 GMT
https://trac.sagemath.org/ticket/13215#comment:131
https://trac.sagemath.org/ticket/13215#comment:131
<blockquote class="citation">
<p>
OK, stupid copyright stuff leading to an uncomfortable discussion: I think the copyright name should be retained as Xavier Caruso. Arpit should add his name in the list of AUTHORS as he has done (but he should of course fix the rest of the banner).
</p>
</blockquote>
<p>
Yes, I agree.
</p>
TicketgitThu, 04 Aug 2016 09:24:29 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:132
https://trac.sagemath.org/ticket/13215#comment:132
<ul>
<li><strong>commit</strong>
changed from <em>f563aba916b4155a9485c46ebbbdfe3404a2c8f1</em> to <em>26e46890a9e6dff7f0f6f69ce7a9bf28f88bc3de</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=26e46890a9e6dff7f0f6f69ce7a9bf28f88bc3de"><span class="icon"></span>26e4689</a></td><td><code>modified copyright banner</code>
</td></tr></table>
TicketarpitdmThu, 04 Aug 2016 09:27:43 GMT
https://trac.sagemath.org/ticket/13215#comment:133
https://trac.sagemath.org/ticket/13215#comment:133
<p>
Hello,
</p>
<p>
I've updated the copyright banner.
</p>
<p>
@Johan: Sorry for misspelling your name.
</p>
<p>
I've kept the <code>inplace_pow</code> and <code>inplace_rmul</code> methods because they are being used by <code>power_right_mod</code> and <code>power_left_mod</code>. I'm now opening it for review.
</p>
<p>
Best,
Arpit.
</p>
TicketarpitdmThu, 04 Aug 2016 09:28:39 GMTstatus changed
https://trac.sagemath.org/ticket/13215#comment:134
https://trac.sagemath.org/ticket/13215#comment:134
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
TicketdlucasMon, 08 Aug 2016 07:30:00 GMTstatus changed
https://trac.sagemath.org/ticket/13215#comment:135
https://trac.sagemath.org/ticket/13215#comment:135
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Hello,
</p>
<p>
There's still some small issues here:
</p>
<ul><li><code>ValueError</code> line 836 in <code>skew_polynomial_element.pyx</code> does not have any error message to explain
to the user why this exception was raised.
</li></ul><ul><li>There's one old raise statement l.528 in <code>skew_polynomial_ring.py</code> (<code>raise IndexError, "generator n not defined"</code>)
</li></ul><ul><li>Some methods in <code>skew_polynomial_element,pyx</code> are still undocumented. Even if I'm kind of ok with <code>_call_</code> not having a short description, I'm definitely not fine with a constructor (l. 2945) without any description of its input parameters...
</li></ul><p>
Back to <code>needs_work</code>.
</p>
<p>
David
</p>
TicketgitMon, 08 Aug 2016 10:02:47 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:136
https://trac.sagemath.org/ticket/13215#comment:136
<ul>
<li><strong>commit</strong>
changed from <em>26e46890a9e6dff7f0f6f69ce7a9bf28f88bc3de</em> to <em>88196bfd70fca121e6e343a03ff2c3119254a671</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=88196bfd70fca121e6e343a03ff2c3119254a671"><span class="icon"></span>88196bf</a></td><td><code>some more fixes to the documentation.</code>
</td></tr></table>
TicketarpitdmMon, 08 Aug 2016 10:06:21 GMT
https://trac.sagemath.org/ticket/13215#comment:137
https://trac.sagemath.org/ticket/13215#comment:137
<blockquote class="citation">
<ul><li><code>ValueError</code> line 836 in <code>skew_polynomial_element.pyx</code> does not have any error message to explain
to the user why this exception was raised.
</li></ul><ul><li>There's one old raise statement l.528 in <code>skew_polynomial_ring.py</code> (<code>raise IndexError, "generator n not defined"</code>)
</li></ul><ul><li>Some methods in <code>skew_polynomial_element,pyx</code> are still undocumented. Even if I'm kind of ok with <code>_call_</code> not having a short description, I'm definitely not fine with a constructor (l. 2945) without any description of its input parameters...
</li></ul></blockquote>
<p>
My apologies for missing out on these. I've made the changes now.
Back to <code>needs_review</code>. :)
</p>
TicketarpitdmMon, 08 Aug 2016 10:06:39 GMTstatus changed
https://trac.sagemath.org/ticket/13215#comment:138
https://trac.sagemath.org/ticket/13215#comment:138
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
TicketdlucasTue, 09 Aug 2016 09:53:21 GMT
https://trac.sagemath.org/ticket/13215#comment:139
https://trac.sagemath.org/ticket/13215#comment:139
<p>
Hello,
</p>
<p>
It seems good to me.
I just have two remarks:
</p>
<ul><li>You wrote error messages for every exception, including <code>ZeroDivisionError</code> and <code>NotImplementedError</code>. IMHO, it's not necessary to write (for instance) <code>ZeroDivisionError("division by zero is not valid")</code>. To be honest, I would have been fine with it... But you wrote <code>ZeroDivisionError: division by zero is not valid</code> which I think is not compatible with Python 3 (not sure though).
</li></ul><ul><li>There's a broken doctest:
<pre class="wiki">File "../rings/polynomial/rings.py", line 744, in sage.rings.polynomial.rings.Rings.ParentMethods.__getitem__
Failed example:
k['x',Frob]
Expected:
Skew Polynomial Ring in x over Finite Field in t of size 5^3 twisted by Frobenius Endomorphism x |-> x^5 on Finite Field in t of size 5^3
Got:
Skew Polynomial Ring in x over Finite Field in t of size 5^3 twisted by t |--> t^5
</pre></li></ul><p>
Apart from that, I'm fine with your changes, tests pass and a lot of work have been done here. Issues found by reviewers have been fixed, and if no one objects, I'll set this to <code>positive_review</code> on the next iteration.
</p>
<p>
Best,
</p>
<p>
David
</p>
TicketarpitdmTue, 09 Aug 2016 10:11:30 GMT
https://trac.sagemath.org/ticket/13215#comment:140
https://trac.sagemath.org/ticket/13215#comment:140
<blockquote class="citation">
<ul><li>You wrote error messages for every exception, including <code>ZeroDivisionError</code> and <code>NotImplementedError</code>. IMHO, it's not necessary to write (for instance) <code>ZeroDivisionError("division by zero is not valid")</code>. To be honest, I would have been fine with it... But you wrote <code>ZeroDivisionError: division by zero is not valid</code> which I think is not compatible with Python 3 (not sure though).
</li></ul></blockquote>
<p>
I looked up online if it was required to write an error message along with <code>ZeroDivisionError</code> since it seems pretty obvious what the error is. It seems, that depending on the situation, this error can mean different things and people write "infinity", "not valid" or simply "division by zero" as an error message after raising the error (python 3 <a class="ext-link" href="https://docs.python.org/3/tutorial/errors.html"><span class="icon"></span>docs</a>). And my thinking behind adding an error message for <code>NotImplementedError</code> is to allow the user to understand the reason behind this not being implemented. Should I remove them?
</p>
TicketdlucasTue, 09 Aug 2016 10:24:32 GMT
https://trac.sagemath.org/ticket/13215#comment:141
https://trac.sagemath.org/ticket/13215#comment:141
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:140" title="Comment 140">arpitdm</a>:
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>You wrote error messages for every exception, including <code>ZeroDivisionError</code> and <code>NotImplementedError</code>. IMHO, it's not necessary to write (for instance) <code>ZeroDivisionError("division by zero is not valid")</code>. To be honest, I would have been fine with it... But you wrote <code>ZeroDivisionError: division by zero is not valid</code> which I think is not compatible with Python 3 (not sure though).
</li></ul></blockquote>
<p>
I looked up online if it was required to write an error message along with <code>ZeroDivisionError</code> since it seems pretty obvious what the error is. It seems, that depending on the situation, this error can mean different things and people write "infinity", "not valid" or simply "division by zero" as an error message after raising the error (python 3 <a class="ext-link" href="https://docs.python.org/3/tutorial/errors.html"><span class="icon"></span>docs</a>). And my thinking behind adding an error message for <code>NotImplementedError</code> is to allow the user to understand the reason behind this not being implemented. Should I remove them?
</p>
</blockquote>
<p>
Oh, ok I understand. If it is advised in Python documentation, please do not revert what you did <code>:)</code>.
</p>
<p>
David
</p>
TicketarpitdmTue, 09 Aug 2016 11:26:22 GMT
https://trac.sagemath.org/ticket/13215#comment:142
https://trac.sagemath.org/ticket/13215#comment:142
<blockquote class="citation">
<ul><li>There's a broken doctest:
<pre class="wiki">File "../rings/polynomial/rings.py", line 744, in sage.rings.polynomial.rings.Rings.ParentMethods.__getitem__
Failed example:
k['x',Frob]
Expected:
Skew Polynomial Ring in x over Finite Field in t of size 5^3 twisted by Frobenius Endomorphism x |-> x^5 on Finite Field in t of size 5^3
Got:
Skew Polynomial Ring in x over Finite Field in t of size 5^3 twisted by t |--> t^5
</pre></li></ul></blockquote>
<p>
There isn't a <code>rings.py</code> file in <code>sage/rings/polynomial</code>. That was moved to <code>sage/categories</code> and the doctest passes there. Maybe you have some portion of an older version of this ticket?
</p>
TicketdlucasTue, 09 Aug 2016 11:52:30 GMT
https://trac.sagemath.org/ticket/13215#comment:143
https://trac.sagemath.org/ticket/13215#comment:143
<p>
Mmh maybe... But it's weird though because I recompiled 7.3 a few days ago, then made a branch and pulled your branch from <a class="closed ticket" href="https://trac.sagemath.org/ticket/13215" title="enhancement: Skew polynomials (closed: fixed)">#13215</a>. I have the latest version of this ticket on my local branch (commit 88196bf) and there's a file <code>rings.py</code> in <code>rings/polynomial</code>.
Even with my clean version of 7.3 as it's available from download (I always keep a clean version of the <code>develop</code> branch), I can find such a file...
</p>
<p>
Anyway, as <code>git log rings/polynomial/rings.py</code> returns nothing while <code>git log categories/rings.py</code> returns your commits, I probably messed up somewhere.
</p>
<p>
So, I guess we're good to go now.
If no one objects, I'll set this ticket to positive review.
</p>
<p>
David
</p>
TicketdlucasWed, 10 Aug 2016 12:32:54 GMTstatus changed
https://trac.sagemath.org/ticket/13215#comment:144
https://trac.sagemath.org/ticket/13215#comment:144
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
As no one objected, I set this ticket to positive review.
</p>
<p>
Best,
</p>
<p>
David
</p>
TicketjsrnThu, 11 Aug 2016 19:12:05 GMTstatus changed
https://trac.sagemath.org/ticket/13215#comment:145
https://trac.sagemath.org/ticket/13215#comment:145
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Sorry to object, but I would like to re-insist on adding the experimental decorator on construction of skew polynomials. The reason is that I'm right now talking to a researcher in skew polynomials (Felice Manganiello), and he is making me aware of considerations that might mean some restructuring down the road. In particular, his natural notion of evaluation is *not* operator evaluation, but rather right (resp. left) modulo of <code>x-a</code>.
</p>
<p>
Therefore, it is not clear that the syntax <code>f(a)</code> should mean operator evaluation. Perhaps it should be e.g. <code>f(a, 'operator')</code> etc. or something different. In any case, it demonstrates that this module is maybe not as mature as we had initially assumed.
</p>
<p>
Best,
Johan
</p>
TicketarpitdmThu, 11 Aug 2016 20:08:23 GMT
https://trac.sagemath.org/ticket/13215#comment:146
https://trac.sagemath.org/ticket/13215#comment:146
<p>
Hi,
</p>
<blockquote class="citation">
<p>
Sorry to object, but I would like to re-insist on adding the experimental decorator on construction of skew polynomials. The reason is that I'm right now talking to a researcher in skew polynomials (Felice Manganiello), and he is making me aware of considerations that might mean some restructuring down the road. In particular, his natural notion of evaluation is *not* operator evaluation, but rather right (resp. left) modulo of <code>x-a</code>.
</p>
</blockquote>
<p>
This reminds me of the three approaches to evaluating a skew polynomial we discussed once before. One of them was, given a skew polynomial <code>p</code> and a point <code>a</code>, <code>p(a) = p.right_quo_rem(x-a)</code> (resp. left). Which I believe is what you are referring to? I think we now have the features to support this generalization of the remainder evaluation method in the non-commutative case. Or perhaps, we add an option to the <code>call</code> method. It takes as argument the algorithm "remainder" (or "operator") and default is set to one of them. That way the third approach of using normal basis can also be easily added if need be in the future.
</p>
<p>
Best,
Arpit.
</p>
TickettscrimThu, 11 Aug 2016 21:32:19 GMTmilestone changed
https://trac.sagemath.org/ticket/13215#comment:147
https://trac.sagemath.org/ticket/13215#comment:147
<ul>
<li><strong>milestone</strong>
changed from <em>sage-7.3</em> to <em>sage-7.4</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:145" title="Comment 145">jsrn</a>:
</p>
<blockquote class="citation">
<p>
Sorry to object, but I would like to re-insist on adding the experimental decorator on construction of skew polynomials. The reason is that I'm right now talking to a researcher in skew polynomials (Felice Manganiello), and he is making me aware of considerations that might mean some restructuring down the road. In particular, his natural notion of evaluation is *not* operator evaluation, but rather right (resp. left) modulo of <code>x-a</code>.
</p>
<p>
Therefore, it is not clear that the syntax <code>f(a)</code> should mean operator evaluation. Perhaps it should be e.g. <code>f(a, 'operator')</code> etc. or something different. In any case, it demonstrates that this module is maybe not as mature as we had initially assumed.
</p>
</blockquote>
<p>
The only thing I would agree with being marked experimental in this case would be the evaluation (i.e., <code>__call__</code>), which would be the only thing that would really see this (behavioral) change. I also think we should take the time to get this consistent with the conventions of skew polynomial research. Could you give Felice access to the code to experiment with (in particular, to get feedback on what additional features he would want and bottlenecks)?
</p>
TickettscrimThu, 11 Aug 2016 22:39:52 GMT
https://trac.sagemath.org/ticket/13215#comment:148
https://trac.sagemath.org/ticket/13215#comment:148
<p>
Did anyone look at the compiled documentation? I highly doubt the pdf documentation would build because <code>\base_ring_endomorphism</code> is not valid latex.
</p>
<p>
Looking more closely, there are also some incorrect statements about the code and many overly verbose statements (e.g., the definition of an automorphism). I'm going over the doc and code now trying to clean things up. However, you currently have an issue in that the skew polynomials themselves do not pickle (which was caught by a <code>TestSuite(S).run()</code> test I'm adding). Feel free to push changes and I can merge them into my branch once this is fixed.
</p>
TicketvbraunThu, 11 Aug 2016 22:43:36 GMT
https://trac.sagemath.org/ticket/13215#comment:149
https://trac.sagemath.org/ticket/13215#comment:149
<p>
PDF docs don't build
</p>
TicketdlucasFri, 12 Aug 2016 06:35:08 GMT
https://trac.sagemath.org/ticket/13215#comment:150
https://trac.sagemath.org/ticket/13215#comment:150
<p>
My mistake, I did not try to compile the pdf documentation, only the html one...
I'll check this as well from now on.
</p>
<p>
David
</p>
TicketgitFri, 12 Aug 2016 08:17:40 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:151
https://trac.sagemath.org/ticket/13215#comment:151
<ul>
<li><strong>commit</strong>
changed from <em>88196bfd70fca121e6e343a03ff2c3119254a671</em> to <em>eb78e694bde02c73c14aae8fb2238b49ef5bba8a</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=eb78e694bde02c73c14aae8fb2238b49ef5bba8a"><span class="icon"></span>eb78e69</a></td><td><code>small fixes to docstrings</code>
</td></tr></table>
TickettscrimFri, 12 Aug 2016 08:28:26 GMT
https://trac.sagemath.org/ticket/13215#comment:152
https://trac.sagemath.org/ticket/13215#comment:152
<p>
I often forget to also check that the pdf docs build, but you can usually see bad latex formatting on the html docs IIRC.
</p>
<p>
Let me also clarify my last comment about pushing, I would prefer pushed changes that fix the pickling. I'm taking care of the docstrings and appreciate not having to deal with trivial merge conflicts.
</p>
TicketarpitdmFri, 12 Aug 2016 08:30:49 GMT
https://trac.sagemath.org/ticket/13215#comment:153
https://trac.sagemath.org/ticket/13215#comment:153
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:148" title="Comment 148">tscrim</a>:
</p>
<blockquote class="citation">
<p>
Did anyone look at the compiled documentation? I highly doubt the pdf documentation would build because <code>\base_ring_endomorphism</code> is not valid latex.
</p>
</blockquote>
<p>
My mistake too. I did not check for pdf docs. They build now.
</p>
<blockquote class="citation">
<p>
Looking more closely, there are also some incorrect statements about the code and many overly verbose statements (e.g., the definition of an automorphism). I'm going over the doc and code now trying to clean things up. However, you currently have an issue in that the skew polynomials themselves do not pickle (which was caught by a <code>TestSuite(S).run()</code> test I'm adding). Feel free to push changes and I can merge them into my branch once this is fixed.
</p>
</blockquote>
<p>
Can you explain a little more about pickling? Or maybe direct me to a link that does?
</p>
TickettscrimFri, 12 Aug 2016 08:36:11 GMT
https://trac.sagemath.org/ticket/13215#comment:154
https://trac.sagemath.org/ticket/13215#comment:154
<p>
Pickling is how Python allows you to save data. In particular, you want
</p>
<pre class="wiki">sage: loads(dumps(x)) == x
True
</pre><p>
for (essentially) any object <code>x</code>.
</p>
TicketarpitdmFri, 12 Aug 2016 10:29:17 GMT
https://trac.sagemath.org/ticket/13215#comment:155
https://trac.sagemath.org/ticket/13215#comment:155
<p>
Hi,
</p>
<blockquote class="citation">
<p>
Pickling is how Python allows you to save data. In particular, you want
</p>
<pre class="wiki">sage: loads(dumps(x)) == x
True
</pre><p>
for (essentially) any object <code>x</code>.
</p>
</blockquote>
<p>
So I ran some examples and here's what I have so far. When the base ring is a finite field of the form <code>GF(q^m)</code>, pickling succeeds, but unpickling does not. Specifically, it fails to recognize or coerce the exponent <code>m</code> as an integer, I think. I tried pickling various other skew polynomials over Integer Ring, Rational Field, etc. and it works just fine. I don't understand where the problem is coming from. Also,
</p>
<pre class="wiki">sage: k.<t> = GF(5^3)
sage: Frob = k.frobenius_endomorphism()
sage: S.<x> = k['x', Frob]
sage: loads(dumps(S)) == S
Traceback:
...
ValueError: (ValueError('exponent must be an integer',), <function SkewPolynomialRing at 0x7f02ca186140>, (Finite Field in t of size 5^3, Identity endomorphism of Finite Field in t of size 5^3, 'x', False))
sage: loads(dumps(k)) == k
True
sage: loads(dumps(Frob)) == Frob
False
</pre>
TickettscrimFri, 12 Aug 2016 12:42:07 GMT
https://trac.sagemath.org/ticket/13215#comment:156
https://trac.sagemath.org/ticket/13215#comment:156
<p>
The bigger problem is that a skew polynomial itself does not pickle:
</p>
<pre class="wiki">sage: R.<t> = ZZ[]
sage: sigma = R.hom([t+1])
sage: S.<x> = SkewPolynomialRing(R, sigma)
sage: loads(dumps(x))
---------------------------------------------------------------------------
PicklingError Traceback (most recent call last)
...
PicklingError: Can't pickle <type 'dictproxy'>: attribute lookup __builtin__.dictproxy failed
</pre><p>
With that being said, the above pickling issue with the Frobenius endomorphism is a separate issue that necessitates a ticket:
</p>
<pre class="wiki">sage: loads(dumps(Frob))
Identity endomorphism of Finite Field in t of size 5^3
</pre><p>
in addition to this independent issue:
</p>
<pre class="wiki">sage: Frob.parent() is Hom(k, k)
True
sage: Hom(k, k)(Frob)
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
...
TypeError: unable to coerce <type 'sage.rings.finite_rings.hom_finite_field_givaro.FrobeniusEndomorphism_givaro'>
</pre>
TicketarpitdmFri, 12 Aug 2016 13:13:21 GMT
https://trac.sagemath.org/ticket/13215#comment:157
https://trac.sagemath.org/ticket/13215#comment:157
<p>
Yes. I was able to recreate this error earlier myself. But I figured that the correct pickling of the ring was dependent on the correct pickling of the underlying automorphism and every other attribute for that matter.
</p>
<p>
How can I resolve these coercion errors? Any pointers would be really helpful. I have no idea where these stem from.
</p>
<p>
Best,
Arpit.
</p>
TickettscrimFri, 12 Aug 2016 13:24:42 GMT
https://trac.sagemath.org/ticket/13215#comment:158
https://trac.sagemath.org/ticket/13215#comment:158
<p>
Well, the pickling issue doesn't stem from coercion, nor does it have to do with the pickling of the parent (which worked okay). It is likely that you need to implement a <code>__reduce__</code> method.
</p>
<p>
The <em>conversion</em> failure of <code>Frob</code> above is likely coming from the generic code for <code>Homset</code> being too strict, but as I said, this is an independent issue.
</p>
TicketgitFri, 12 Aug 2016 20:43:49 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:159
https://trac.sagemath.org/ticket/13215#comment:159
<ul>
<li><strong>commit</strong>
changed from <em>eb78e694bde02c73c14aae8fb2238b49ef5bba8a</em> to <em>ec196113af6d216fb1fdeb1896eca677213d0510</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=ec196113af6d216fb1fdeb1896eca677213d0510"><span class="icon"></span>ec19611</a></td><td><code>added a reduce method to support pickling and unpickling of skew polynomials</code>
</td></tr></table>
TicketarpitdmFri, 12 Aug 2016 20:49:51 GMT
https://trac.sagemath.org/ticket/13215#comment:160
https://trac.sagemath.org/ticket/13215#comment:160
<p>
I've added a <code>__reduce__</code> method that pickles and unpickles a skew polynomial and a skew polynomial ring over a non-finite base ring correctly now. The error in the earlier example you mentioned is now resolved. However, the problem with the finite fields case persists. I haven't managed to swat that bug away yet.
Any ideas on what is causing that?
</p>
TickettscrimFri, 12 Aug 2016 23:46:45 GMT
https://trac.sagemath.org/ticket/13215#comment:161
https://trac.sagemath.org/ticket/13215#comment:161
<p>
The (un)pickling of the Frobenius (well, perhaps its base class) is not setting the <code>_power</code> correctly:
</p>
<pre class="wiki">sage: Frob.power()
1
sage: loads(dumps(Frob)).power()
0
</pre><p>
The solution is probably to make the <code>__reduce__</code> less generic. This will need to be a separate ticket (which we don't have to make as a dependency of this one).
</p>
TicketjsrnSat, 13 Aug 2016 09:32:17 GMT
https://trac.sagemath.org/ticket/13215#comment:162
https://trac.sagemath.org/ticket/13215#comment:162
<p>
About the experimental warning: @arpitdm: yes, what you describe is exactly the other evaluation notions, so it is not complicated to implement them - it is only a matter of discussing interface. For progressing with our GSoC, I strongly prefer to take that discussion in a follow-up ticket (e.g. during SD75) and just mark skew polynomials with the experimental warning.
</p>
<p>
@tscrim: Marking *only* <code>__call__</code> is certainly an option, and will be less disconcerting to users who wants to take skew polynomials into use soon, and I am also sort of OK with this. However, I prefer to mark the entire module as experimental: this <code>__call__</code> issue might just be the first example of other similar conventions. Felice has already asked for a trac account and will look at the functionality when he has time. But in the interest of being able to continue with our other goals, I would strongly prefer to get this major ticket closed before we are sure everything has converged, and the point of the experimental decorator is exactly to be able to do that.
</p>
<p>
Best,
Johan
</p>
TicketjsrnSat, 13 Aug 2016 09:46:27 GMTbranch changed
https://trac.sagemath.org/ticket/13215#comment:163
https://trac.sagemath.org/ticket/13215#comment:163
<ul>
<li><strong>branch</strong>
changed from <em>u/arpitdm/skew_polynomials</em> to <em>u/jsrn/skew_polynomials</em>
</li>
</ul>
TicketjsrnSat, 13 Aug 2016 09:55:48 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:164
https://trac.sagemath.org/ticket/13215#comment:164
<ul>
<li><strong>commit</strong>
changed from <em>ec196113af6d216fb1fdeb1896eca677213d0510</em> to <em>f78efb1e18976aefa66f3a1937e92d9c571c4293</em>
</li>
</ul>
<p>
I renamed <code>power_left_mod</code> to <code>left_power_mod</code> similarly for <code>right</code>. I suggested the old names myself, but realised that it's much better if all such sided operations begin with left/right (and all the other sided operations already do).
</p>
<p>
Best,
Johan
</p>
<hr />
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=15ac35033531cf2d87eb999b3447b7f8affb301b"><span class="icon"></span>15ac350</a></td><td><code>Merge branch 'u/arpitdm/skew_polynomials' of git://trac.sagemath.org/sage into 13215_skew_polynomials</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=f78efb1e18976aefa66f3a1937e92d9c571c4293"><span class="icon"></span>f78efb1</a></td><td><code>power_left/right_mod -> left/right_power_mod</code>
</td></tr></table>
TicketarpitdmSat, 13 Aug 2016 12:06:07 GMT
https://trac.sagemath.org/ticket/13215#comment:165
https://trac.sagemath.org/ticket/13215#comment:165
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:162" title="Comment 162">jsrn</a>:
</p>
<blockquote class="citation">
<p>
About the experimental warning: @arpitdm: yes, what you describe is exactly the other evaluation notions, so it is not complicated to implement them - it is only a matter of discussing interface. For progressing with our GSoC, I strongly prefer to take that discussion in a follow-up ticket (e.g. during SD75) and just mark skew polynomials with the experimental warning.
</p>
</blockquote>
<p>
Agreed. We can keep that for SD75.
</p>
<blockquote class="citation">
<p>
@tscrim: Marking *only* <code>__call__</code> is certainly an option, and will be less disconcerting to users who wants to take skew polynomials into use soon, and I am also sort of OK with this. However, I prefer to mark the entire module as experimental: this <code>__call__</code> issue might just be the first example of other similar conventions. Felice has already asked for a trac account and will look at the functionality when he has time. But in the interest of being able to continue with our other goals, I would strongly prefer to get this major ticket closed before we are sure everything has converged, and the point of the experimental decorator is exactly to be able to do that.
</p>
</blockquote>
<p>
If we are placing the experimental decorator, which I think we should, I second placing it over the entire skew polynomial ticket. Because <code>call</code> is not the only one that will be changed. Derivations are missing. Anyone who is working on skew polynomial research will probably point that out immediately. And there could be a bunch of changes to accommodate that.
</p>
TickettscrimSat, 13 Aug 2016 14:18:41 GMTcommit, branch changed
https://trac.sagemath.org/ticket/13215#comment:166
https://trac.sagemath.org/ticket/13215#comment:166
<ul>
<li><strong>commit</strong>
changed from <em>f78efb1e18976aefa66f3a1937e92d9c571c4293</em> to <em>92ae069efadad32431ebd83ffa6dd42ccb291b4e</em>
</li>
<li><strong>branch</strong>
changed from <em>u/jsrn/skew_polynomials</em> to <em>public/rings/skew_polynomials-13215</em>
</li>
</ul>
<p>
I've gone through and made some cleanup to the documentation and code. I've made the <code>SkewPolynomial</code> actually behave like an ABC that can be subclassed to sparse skew polynomials (mainly, removed the <code>_coeffs</code> attribute).
</p>
<p>
I strongly disagree with marking the entire class with <code>@experimental</code>. For example, all of the changes I did would not require any deprecation, i.e., the public API and behavior was unchanged. If you know you must change the API/behavior to accommodate something, then this code has no business being in Sage and you should just go ahead and do it. If the behavior could undergo (radical) changes, then the code isn't (IMO) correct and should not be included.
</p>
<p>
(Personally, I see very little use-cases for <code>@experimental</code> and no real use at the top-level. People who will actually experiment with the code will be those who can just get the code from the trac server (which requires no login). By marking something at the top-level, nobody will really use the code because there are no guarantees.)
</p>
<hr />
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=11fc0284d585af474097684295ef096d0846276b"><span class="icon"></span>11fc028</a></td><td><code>Merge branch 'u/arpitdm/skew_polynomials' of trac.sagemath.org:sage into public/rings/skew_polynomials-13215</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=c2bfcd7a8394c242e8182dc2411769e3b5843b70"><span class="icon"></span>c2bfcd7</a></td><td><code>Merge branch 'u/arpitdm/skew_polynomials' of trac.sagemath.org:sage into public/rings/skew_polynomials-13215</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=225a66a90974b1eab25119831d23c98e16c7a5ce"><span class="icon"></span>225a66a</a></td><td><code>Initial round of docstring and code fixing.</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=05587a9af6623225126c1b3b0b881e69d034e195"><span class="icon"></span>05587a9</a></td><td><code>Merge branch 'u/jsrn/skew_polynomials' of trac.sagemath.org:sage into public/rings/skew_polynomials-13215</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=92ae069efadad32431ebd83ffa6dd42ccb291b4e"><span class="icon"></span>92ae069</a></td><td><code>Moving things around so the ABC actually is the ABC for univariate skew polynomials.</code>
</td></tr></table>
TickettscrimSat, 13 Aug 2016 14:28:35 GMTstatus changed
https://trac.sagemath.org/ticket/13215#comment:167
https://trac.sagemath.org/ticket/13215#comment:167
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
TicketjsrnSun, 14 Aug 2016 10:09:36 GMT
https://trac.sagemath.org/ticket/13215#comment:168
https://trac.sagemath.org/ticket/13215#comment:168
<p>
Hi Travis,
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:166" title="Comment 166">tscrim</a>:
</p>
<blockquote class="citation">
<p>
I've gone through and made some cleanup to the documentation and code. I've made the <code>SkewPolynomial</code> actually behave like an ABC that can be subclassed to sparse skew polynomials (mainly, removed the <code>_coeffs</code> attribute).
</p>
</blockquote>
<p>
Great!
</p>
<blockquote class="citation">
<p>
I strongly disagree with marking the entire class with <code>@experimental</code>. For example, all of the changes I did would not require any deprecation, i.e., the public API and behavior was unchanged. If you know you must change the API/behavior to accommodate something, then this code has no business being in Sage and you should just go ahead and do it. If the behavior could undergo (radical) changes, then the code isn't (IMO) correct and should not be included.
</p>
<p>
(Personally, I see very little use-cases for <code>@experimental</code> and no real use at the top-level. People who will actually experiment with the code will be those who can just get the code from the trac server (which requires no login). By marking something at the top-level, nobody will really use the code because there are no guarantees.)
</p>
</blockquote>
<p>
I see we disagree on the <code>@experimental</code> decorator itself. It is a "local beta", in that it says: "you can count on this functionality being in Sage soon in roughly the same shape. If you want to use it already, be prepared that the API might change mildly". It facilitates Request-for-comments from developers who believe they have well-designed some module, but acknowledge that they might not have considered everything. This goes doubly for math code, since any given math structure can be considered from so many angles, and usually the initial developer considers mostly just one. It allows building other new stuff on top of it (which we really need now: <a class="closed ticket" href="https://trac.sagemath.org/ticket/20970" title="enhancement: Gabidulin Codes (closed: fixed)">#20970</a>), which postponing the ticket wouldn't, and that new stuff might not need an <code>@experimental</code> warning itself (for <a class="closed ticket" href="https://trac.sagemath.org/ticket/20970" title="enhancement: Gabidulin Codes (closed: fixed)">#20970</a>, skew polynomials is just a tool, and its exact API has roughly no bearing on the API of the Gabidulin codes).
</p>
<p>
The decorator was originally suggested because without it, it is difficult to start introducing the "basic" modules on a new set of functionality, since one can only really judge its design once many things have been developed on top of it. This happened for us in coding theory, where we felt forced to have our own fork of Sage and develop for nine months before making the first pushes to Sage, greatly complicating the development process.
</p>
<p>
See also the discussion here:
<a class="ext-link" href="https://groups.google.com/forum/#!topic/sage-devel/LXWs6KOw0Lk"><span class="icon"></span>https://groups.google.com/forum/#!topic/sage-devel/LXWs6KOw0Lk</a>.
This ticket is exactly one of the intended use cases for this decorator.
</p>
<p>
I still insist on the module-wide experimental warning here. I expect I will be active in polishing the skew polynomial module in the coming months, and I really don't want to be burdened/limited by deprecation considerations all the time. On the other hand, we really need this ticket to go into Sage to proceed with the GSoC, and at least >95% of the current functionality and API is good.
</p>
TickettscrimSun, 14 Aug 2016 13:04:59 GMT
https://trac.sagemath.org/ticket/13215#comment:169
https://trac.sagemath.org/ticket/13215#comment:169
<p>
From the discussion above, there are suggestions of <em>major</em> behavioral and API changes, not mild. While this is not the hill I necessarily want to die on, I do have a use-case for this code. By saying the module is "subject to change without notice" means I can't build upon it because it could break all of my code, as well as yours for <a class="closed ticket" href="https://trac.sagemath.org/ticket/20970" title="enhancement: Gabidulin Codes (closed: fixed)">#20970</a>. You say you only need this as a tool, but suppose <a class="closed ticket" href="https://trac.sagemath.org/ticket/20970" title="enhancement: Gabidulin Codes (closed: fixed)">#20970</a> is merged before this is stable and then you force whomever has to make changes to this code to also fix your code (which defeats the entire point) or your code gets broken if it is something not doctested.
</p>
<p>
You also seem stuck on the idea the code has to be merged in order to proceed with GSoC. You can build other code on top of this, and this doesn't need to be merged by the end of the program for the GSoC to be considered successful (granted, it took a little too long for the knot theory GSoC code to get merged).
</p>
<p>
Additionally, if 95% of the code is set, then mark the 5% with <code>@experimental</code> if you think it is that likely to change. If you disagree, then I am forced to disagree with your assessment of the completeness of the code. As a reminder, <em>internal</em> changes do not need to be deprecated (for example, none of the changes I pushed would not require one).
</p>
TickettscrimSun, 14 Aug 2016 13:05:43 GMT
https://trac.sagemath.org/ticket/13215#comment:170
https://trac.sagemath.org/ticket/13215#comment:170
<p>
If you feel we are at an impasse, then we can ask on sage-devel.
</p>
TicketjsrnSun, 14 Aug 2016 16:51:35 GMT
https://trac.sagemath.org/ticket/13215#comment:171
https://trac.sagemath.org/ticket/13215#comment:171
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:169" title="Comment 169">tscrim</a>:
</p>
<blockquote class="citation">
<p>
From the discussion above, there are suggestions of <em>major</em> behavioral and API changes, not mild.
</p>
</blockquote>
<p>
I don't consider the <code>__call__</code> discussion a major change. My current idea for accommodating multiple notions of evaluation would be to throw a warning/error when <code>__call__</code> is used before "setting" the default evaluation on the skew polynomial ring in question. That's a completely local change (local to <code>__call__</code>. But as I mentioned, I prefer to postpone that discussion to a later ticket.
</p>
<blockquote class="citation">
<p>
While this is not the hill I necessarily want to die on, I do have a use-case for this code. By saying the module is "subject to change without notice" means I can't build upon it because it could break all of my code, as well as yours for <a class="closed ticket" href="https://trac.sagemath.org/ticket/20970" title="enhancement: Gabidulin Codes (closed: fixed)">#20970</a>. You say you only need this as a tool, but suppose <a class="closed ticket" href="https://trac.sagemath.org/ticket/20970" title="enhancement: Gabidulin Codes (closed: fixed)">#20970</a> is merged before this is stable and then you force whomever has to make changes to this code to also fix your code (which defeats the entire point) or your code gets broken if it is something not doctested.
</p>
</blockquote>
<p>
As I said, I imagine that it would be an overlapping set of developers (at least me) who would improve skew polynomials in the forseeable future, as well as use it before it was "stable", in e.g. <a class="closed ticket" href="https://trac.sagemath.org/ticket/20970" title="enhancement: Gabidulin Codes (closed: fixed)">#20970</a>. This is, once again, what I would regard the common use-case of <code>@experimental</code> (whoever adds some feature usually needs it him/herself).
</p>
<p>
If you want to build upon this code yourself, I also don't understand your stance: not having <code>@experimental</code> means that any near-future sensible change to this module's API will be clumsy to accommodate. If you have made use of this module in the mean-time, either included in Sage or not, there's two possible outcomes: the improvement is not done, or it is done and your code will start spewing warnings. I don't see how either outcomes are better than what would happen with the <code>@experimental</code> warning on.
</p>
<blockquote class="citation">
<p>
You also seem stuck on the idea the code has to be merged in order to proceed with GSoC. You can build other code on top of this, and this doesn't need to be merged by the end of the program for the GSoC to be considered successful (granted, it took a little too long for the knot theory GSoC code to get merged).
</p>
</blockquote>
<p>
You know how it is: unmerged code just has a much larger probability of rotting on trac or ending up causing a huge workload for the mentors/other interessants of the project. We already have a queue of 4 tickets waiting for this one, and I really want to pop most of them before GSoC runs out!
</p>
<blockquote class="citation">
<p>
Additionally, if 95% of the code is set, then mark the 5% with <code>@experimental</code> if you think it is that likely to change.
</p>
</blockquote>
<p>
But I don't *know* what could change! It's my assessment that this is pretty close to stable, and Xavier, Arpit, David, Burcin, you, and me have thought things pretty well through. But it *is* a large, new structure to Sage.
</p>
<blockquote class="citation">
<p>
If you disagree, then I am forced to disagree with your assessment of the completeness of the code. As a reminder, <em>internal</em> changes do not need to be deprecated (for example, none of the changes I pushed would not require one).
</p>
</blockquote>
<p>
I am somewhat shocked at how vehemently you are against the <code>@experimental</code> decorator. Discussion on sage-devel it is, I guess...
</p>
TicketjsrnSun, 14 Aug 2016 17:19:50 GMT
https://trac.sagemath.org/ticket/13215#comment:172
https://trac.sagemath.org/ticket/13215#comment:172
<p>
<a class="ext-link" href="https://groups.google.com/forum/#!topic/sage-devel/JlJXGtxsYr0"><span class="icon"></span>https://groups.google.com/forum/#!topic/sage-devel/JlJXGtxsYr0</a>
</p>
TicketjsrnWed, 17 Aug 2016 08:31:04 GMT
https://trac.sagemath.org/ticket/13215#comment:173
https://trac.sagemath.org/ticket/13215#comment:173
<p>
The discussion on sage-devel seems to have tapered off, so we need to take a decision here. We got two pieces of input: leif suggested a compromise with a warning in the doc, and wstein seemed to favour something like not pre-importing the module until the module is deemed "stable".
</p>
<p>
IMHO, neither of the two shared your concerns of not being able to build upon
the module, and seemed OK with the idea of accepting not-completely-stable code
into Sage. Conversely, they apparently didn't really like @experimental in the current form either.
</p>
<p>
I like both of the alternative suggestions less than just including the module and defining it as stable, handling any possible deprecation warnings that might arise in the near future. However, I still claim that using @experimental on the module is the right way to go.
</p>
<p>
But in the interest of progressing, I leave it to you Travis: if you are still strongly against a module-level @experimental, let's drop it. I guess you are still OK with having @experimental on <span class="underline">call</span>, together with a sensible message in the doc, e.g.
</p>
<pre class="wiki"> WARNING::
Currently only "operator evaluation" of skew polynomials is implemented. There are two other common notions of evaluation of `f(x)` at some `a` from the base ring, namely the remainder after left or right modulo by `x-a`. The current calling convention might change in the future to accommodate these.
</pre><p>
Best,
Johan
</p>
TickettscrimWed, 17 Aug 2016 08:36:45 GMT
https://trac.sagemath.org/ticket/13215#comment:174
https://trac.sagemath.org/ticket/13215#comment:174
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:173" title="Comment 173">jsrn</a>:
</p>
<blockquote class="citation">
<p>
But in the interest of progressing, I leave it to you Travis: if you are still strongly against a module-level @experimental, let's drop it. I guess you are still OK with having @experimental on <span class="underline">call</span>, together with a sensible message in the doc, e.g.
</p>
<pre class="wiki"> WARNING::
Currently only "operator evaluation" of skew polynomials is implemented.
There are two other common notions of evaluation of `f(x)` at some `a`
from the base ring, namely the remainder after left or right modulo by `x-a`.
The current calling convention might change in the future to accommodate these.
</pre></blockquote>
<p>
I still strongly am against a module-level <code>@experimental</code>, but I think this is a good compromise. Positive review on my part once this is done. Thank you for your input and work on this!
</p>
TicketchapotonWed, 17 Aug 2016 11:54:07 GMT
https://trac.sagemath.org/ticket/13215#comment:175
https://trac.sagemath.org/ticket/13215#comment:175
<p>
from the patchbot:
</p>
<pre class="wiki">+inside file: b/src/sage/rings/polynomial/skew_polynomial_ring_constructor.py
+@@ -0,0 +1,207 @@
++ ....: print S
+python2-only print syntax inserted on 1 non-empty lines
+Traceback (most recent call last):
+ File "/nfs4/home6/vdelecro/.local/lib/python2.7/site-packages/sage_patchbot/patchbot.py", line 1053, in test_a_ticket
+ baseline=baseline, **kwds)
+ File "/usr/labri/vdelecro/.local/lib/python2.7/site-packages/sage_patchbot/plugins.py", line 368, in oldstyle_print
+ msg="python2-only print syntax", **kwds)
+ File "/usr/labri/vdelecro/.local/lib/python2.7/site-packages/sage_patchbot/plugins.py", line 214, in exclude_new
+ raise ValueError(full_msg)
+ValueError: python2-only print syntax inserted on 1 non-empty lines
</pre><pre class="wiki">+inside file: b/src/sage/rings/polynomial/skew_polynomial_element.pyx
+@@ -0,0 +1,3019 @@
++ INPUT::
+inside file: b/src/sage/rings/polynomial/skew_polynomial_ring.py
+@@ -0,0 +1,732 @@
++ INPUT::
+Bad Input/Output blocks inserted on 2 non-empty lines
+Traceback (most recent call last):
+ File "/nfs4/home6/vdelecro/.local/lib/python2.7/site-packages/sage_patchbot/patchbot.py", line 1053, in test_a_ticket
+ baseline=baseline, **kwds)
+ File "/usr/labri/vdelecro/.local/lib/python2.7/site-packages/sage_patchbot/plugins.py", line 352, in input_output_block
+ msg="Bad Input/Output blocks", **kwds)
+ File "/usr/labri/vdelecro/.local/lib/python2.7/site-packages/sage_patchbot/plugins.py", line 214, in exclude_new
+ raise ValueError(full_msg)
+ValueError: Bad Input/Output blocks inserted on 2 non-empty lines
</pre>
TicketdlucasWed, 17 Aug 2016 12:11:36 GMT
https://trac.sagemath.org/ticket/13215#comment:176
https://trac.sagemath.org/ticket/13215#comment:176
<p>
Oh, you managed to get some feedback from the patchbot?
I was actually slightly worried about the Python 2 only syntax, but I do not know
any efficient way to test for this locally...
</p>
<p>
David
</p>
TicketarpitdmWed, 17 Aug 2016 16:12:33 GMT
https://trac.sagemath.org/ticket/13215#comment:177
https://trac.sagemath.org/ticket/13215#comment:177
<p>
Based on the conclusions reached above, I tried adding the experimental warning to the call method. And I get an error message while building Sage that seems to that it is not possible to add the experimental decorator at all.
</p>
<pre class="wiki">Error compiling Cython file:
------------------------------------------------------------
...
else:
n = self._new_c([],P)
return n
@experimental(trac_number=13215)
def __call__(self, eval_pt):
^
------------------------------------------------------------
sage/rings/polynomial/skew_polynomial_element.pyx:336:4: special functions of cdef classes cannot have decorators
</pre><p>
Is there an alternate way to add the experimental warning?
</p>
TicketgitWed, 17 Aug 2016 20:49:28 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:178
https://trac.sagemath.org/ticket/13215#comment:178
<ul>
<li><strong>commit</strong>
changed from <em>92ae069efadad32431ebd83ffa6dd42ccb291b4e</em> to <em>4b863456690dd1bc167fcad1621878ca0771abd3</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=4b863456690dd1bc167fcad1621878ca0771abd3"><span class="icon"></span>4b86345</a></td><td><code>fixed INPUT/OUTPUT syntax in docs. updated print statement to python3 syntax</code>
</td></tr></table>
TicketarpitdmWed, 17 Aug 2016 21:02:42 GMT
https://trac.sagemath.org/ticket/13215#comment:179
https://trac.sagemath.org/ticket/13215#comment:179
<p>
I think I've managed to rectify the patchbot errors. How can I check if there are further patchbot errors?
</p>
TicketchapotonThu, 18 Aug 2016 07:20:50 GMT
https://trac.sagemath.org/ticket/13215#comment:180
https://trac.sagemath.org/ticket/13215#comment:180
<p>
Patchbots should soon come back. Then you have to wait for one of them to look at your ticket. Or run your own patchbot.
</p>
TicketjsrnThu, 18 Aug 2016 08:20:55 GMT
https://trac.sagemath.org/ticket/13215#comment:181
https://trac.sagemath.org/ticket/13215#comment:181
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:177" title="Comment 177">arpitdm</a>:
</p>
<blockquote class="citation">
<p>
Based on the conclusions reached above, I tried adding the experimental warning to the call method. And I get an error message while building Sage that seems to that it is not possible to add the experimental decorator at all.
</p>
</blockquote>
<p>
Ah ok, that's a snag. Have you tried looking up the source code for <code>@experimental</code> in <code>sage/misc/superseded.py</code>? You should be able to just print the warning manually as the first line of <code>__call__</code>. Perhaps add a comment mentioning this emulates <code>@experimental</code> such that it can be found if one greps "@experimental".
</p>
<p>
Best,
Johan
</p>
TickettscrimThu, 18 Aug 2016 12:42:46 GMT
https://trac.sagemath.org/ticket/13215#comment:182
https://trac.sagemath.org/ticket/13215#comment:182
<p>
That's right, it's a Cython issue because double-underscore methods need to be assigned to slots at compile time (or at least be handled in a special way), so decorators don't work on them. I agree with <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:181" title="Comment 181">comment:181</a>, just issue the warning yourself.
</p>
TicketgitThu, 18 Aug 2016 13:48:20 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:183
https://trac.sagemath.org/ticket/13215#comment:183
<ul>
<li><strong>commit</strong>
changed from <em>4b863456690dd1bc167fcad1621878ca0771abd3</em> to <em>8553b9349afd40e5ae5d5a26811826b6a647ec5d</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=8553b9349afd40e5ae5d5a26811826b6a647ec5d"><span class="icon"></span>8553b93</a></td><td><code>added a manual experimental warning to the '__call__' method</code>
</td></tr></table>
TicketarpitdmThu, 18 Aug 2016 13:52:10 GMT
https://trac.sagemath.org/ticket/13215#comment:184
https://trac.sagemath.org/ticket/13215#comment:184
<p>
Hello,
</p>
<p>
I've added the experimental warning to the <code>__call__</code> method manually based on the source code for <code>experimental</code> as suggested by @jsrn. There are a couple of doctests that "fail" because they issue the experimental warning before returning the correct answer. Should I modify the doctests or keep them as it is?
</p>
<p>
Best,
Arpit.
</p>
TickettscrimThu, 18 Aug 2016 13:56:54 GMT
https://trac.sagemath.org/ticket/13215#comment:185
https://trac.sagemath.org/ticket/13215#comment:185
<p>
You need to make sure all doctests pass, so you need to include the warning in the doctest output.
</p>
TicketdlucasThu, 18 Aug 2016 13:57:37 GMT
https://trac.sagemath.org/ticket/13215#comment:186
https://trac.sagemath.org/ticket/13215#comment:186
<p>
As warnings are issued only once per session, you can fix this by triggering the experimental warning in a specific "doctest" at the top of the file.
See what have been done in <code>rings/asymptotic/asymptotic_ring.py</code> for instance.
</p>
TicketgitThu, 18 Aug 2016 14:07:23 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:187
https://trac.sagemath.org/ticket/13215#comment:187
<ul>
<li><strong>commit</strong>
changed from <em>8553b9349afd40e5ae5d5a26811826b6a647ec5d</em> to <em>137b58b1a96e9c4830a057b62a8a432bb7bb773f</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=137b58b1a96e9c4830a057b62a8a432bb7bb773f"><span class="icon"></span>137b58b</a></td><td><code>fixed small doctest errors</code>
</td></tr></table>
TicketarpitdmThu, 18 Aug 2016 14:13:34 GMTstatus changed
https://trac.sagemath.org/ticket/13215#comment:188
https://trac.sagemath.org/ticket/13215#comment:188
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Done. Fixed!
I'm opening it for review.
</p>
TicketarpitdmThu, 18 Aug 2016 14:13:57 GMTstatus changed
https://trac.sagemath.org/ticket/13215#comment:189
https://trac.sagemath.org/ticket/13215#comment:189
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
TicketjsrnThu, 18 Aug 2016 17:51:49 GMT
https://trac.sagemath.org/ticket/13215#comment:190
https://trac.sagemath.org/ticket/13215#comment:190
<p>
Is there a reason you didn't add a single doctest at the top for triggering the warning, as David suggested?
</p>
TickettscrimThu, 18 Aug 2016 22:50:11 GMTstatus changed
https://trac.sagemath.org/ticket/13215#comment:191
https://trac.sagemath.org/ticket/13215#comment:191
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
@jsm @dlucas Warnings for doctests are handled different than in a session. For deprecation warnings, these are done every time the deprecation message is hit, and I believe that behavior extends to all warnings.
</p>
<p>
@arpitdm If you are going to do a redirection, which is good for if evaluation does change provided you give it a more descriptive name (and include it as part of the public API), I believe you can just mark the (Python) function with the decorator without problem (as it is no longer a special method).
</p>
TicketdlucasFri, 19 Aug 2016 06:26:49 GMT
https://trac.sagemath.org/ticket/13215#comment:192
https://trac.sagemath.org/ticket/13215#comment:192
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:191" title="Comment 191">tscrim</a>:
</p>
<blockquote class="citation">
<p>
@jsm @dlucas Warnings for doctests are handled different than in a session. For deprecation warnings, these are done every time the deprecation message is hit, and I believe that behavior extends to all warnings.
</p>
</blockquote>
<p>
Is that a recent behaviour? Because triggering the experimental warning once works just fine in the file I referenced above. I also used this myself in <a class="closed ticket" href="https://trac.sagemath.org/ticket/20284" title="enhancement: A class to manage embedding between non-prime fields (closed: fixed)">#20284</a> and it also worked perfectly.
</p>
TicketjsrnFri, 19 Aug 2016 08:37:24 GMT
https://trac.sagemath.org/ticket/13215#comment:193
https://trac.sagemath.org/ticket/13215#comment:193
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:191" title="Comment 191">tscrim</a>:
</p>
<blockquote class="citation">
<p>
@jsm @dlucas Warnings for doctests are handled different than in a session. For deprecation warnings, these are done every time the deprecation message is hit, and I believe that behavior extends to all warnings.
</p>
</blockquote>
<p>
As David, I have my doubts about this too...
</p>
<blockquote class="citation">
<p>
@arpitdm If you are going to do a redirection, which is good for if evaluation does change provided you give it a more descriptive name (and include it as part of the public API), I believe you can just mark the (Python) function with the decorator without problem (as it is no longer a special method).
</p>
</blockquote>
<p>
Agree, since you did an indirection anyway. <code>operator_eval</code> is a more descriptive name. Then the two others that we might add later on could be <code>left_mod_eval</code> and <code>right_mod_eval</code>; not too long, uses very common abbreviations, and all follow the same style.
</p>
TickettscrimFri, 19 Aug 2016 23:12:40 GMT
https://trac.sagemath.org/ticket/13215#comment:194
https://trac.sagemath.org/ticket/13215#comment:194
<p>
It's also quite possible that classes and methods/functions are handled differently by the decorator.
</p>
TicketcarusoTue, 23 Aug 2016 05:21:11 GMT
https://trac.sagemath.org/ticket/13215#comment:195
https://trac.sagemath.org/ticket/13215#comment:195
<p>
Here is another issue:
</p>
<pre class="wiki">sage: k = GF(5^3)
sage: S.<x> = SkewPolynomialRing(k, k.frobenius_endomorphism())
sage: x.right_quo_rem(Graphics())
...
zsh: segmentation fault (core dumped) sage
</pre><p>
As far as understand, one has to use the decorator <code>@coerce_binop</code> for all binary operations.
</p>
TicketarpitdmTue, 23 Aug 2016 05:52:14 GMT
https://trac.sagemath.org/ticket/13215#comment:196
https://trac.sagemath.org/ticket/13215#comment:196
<p>
@tscrim, @jsrn - Just to clarify, there is a <code>__call__</code> magic function for evaluation of the polynomial which calls an <code>_call</code> function. Are you suggesting that I rename the latter to <code>operator_eval</code>?
</p>
<p>
@Caruso - What does Graphics()`
</p>
TicketcarusoTue, 23 Aug 2016 05:55:01 GMT
https://trac.sagemath.org/ticket/13215#comment:197
https://trac.sagemath.org/ticket/13215#comment:197
<p>
Graphics() is an empty plot. It is not a skew polynomials; that's the point.
</p>
TicketcarusoTue, 23 Aug 2016 05:58:40 GMT
https://trac.sagemath.org/ticket/13215#comment:198
https://trac.sagemath.org/ticket/13215#comment:198
<p>
Do you really need the <code>__call__</code> function for this ticket? Otherwise, I would suggest to forget it for now and reintroduce it only when needed.
</p>
TicketjsrnTue, 23 Aug 2016 11:33:58 GMT
https://trac.sagemath.org/ticket/13215#comment:199
https://trac.sagemath.org/ticket/13215#comment:199
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:196" title="Comment 196">arpitdm</a>:
</p>
<blockquote class="citation">
<p>
@tscrim, @jsrn - Just to clarify, there is a <code>__call__</code> magic function for evaluation of the polynomial which calls an <code>_call</code> function. Are you suggesting that I rename the latter to <code>operator_eval</code>?
</p>
</blockquote>
<p>
Yes.
</p>
TicketjsrnTue, 23 Aug 2016 11:35:14 GMT
https://trac.sagemath.org/ticket/13215#comment:200
https://trac.sagemath.org/ticket/13215#comment:200
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:198" title="Comment 198">caruso</a>:
</p>
<blockquote class="citation">
<p>
Do you really need the <code>__call__</code> function for this ticket? Otherwise, I would suggest to forget it for now and reintroduce it only when needed.
</p>
</blockquote>
<p>
We need it for Gabidulin codes (for which we need <a class="closed ticket" href="https://trac.sagemath.org/ticket/21131" title="enhancement: Interpolation, Minimal Vanishing Polynomial and Multi Point Evaluation ... (closed: fixed)">#21131</a>)
</p>
TicketgitTue, 23 Aug 2016 15:17:39 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:201
https://trac.sagemath.org/ticket/13215#comment:201
<ul>
<li><strong>commit</strong>
changed from <em>137b58b1a96e9c4830a057b62a8a432bb7bb773f</em> to <em>2d4a1409011f4dda2bb0bbd2d0c8970e9824dafb</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=cf14817fac0c9aa3d76d350929241e3216639bd7"><span class="icon"></span>cf14817</a></td><td><code>Merge branch 'public/rings/skew_polynomials-13215' of git://trac.sagemath.org/sage into skew_polynomials</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=2d4a1409011f4dda2bb0bbd2d0c8970e9824dafb"><span class="icon"></span>2d4a140</a></td><td><code>triggered experimental warning at the top of the session</code>
</td></tr></table>
TicketarpitdmTue, 23 Aug 2016 15:31:44 GMT
https://trac.sagemath.org/ticket/13215#comment:202
https://trac.sagemath.org/ticket/13215#comment:202
<p>
A helper function <code>def _call</code> has been added that acts as a helper function to accommodate for operator evaluation. The <code>def operator_eval</code> is available as a public method. And the experimental warning is triggered at the top of the module. The segmentation fault due to <code>Graphics()</code> for <code>def right_quo_rem</code> (resp. left) have been resolved.
</p>
TicketgitTue, 23 Aug 2016 18:44:36 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:203
https://trac.sagemath.org/ticket/13215#comment:203
<ul>
<li><strong>commit</strong>
changed from <em>2d4a1409011f4dda2bb0bbd2d0c8970e9824dafb</em> to <em>5dcea53ec25b2cc19873230ce697ec3720d95238</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=5dcea53ec25b2cc19873230ce697ec3720d95238"><span class="icon"></span>5dcea53</a></td><td><code>adding coerce_binop decorator to relevant methods</code>
</td></tr></table>
TicketarpitdmTue, 23 Aug 2016 18:48:41 GMT
https://trac.sagemath.org/ticket/13215#comment:204
https://trac.sagemath.org/ticket/13215#comment:204
<p>
The <code>coerce_binop</code> decorator is added to the relevant methods to resolve the remaining segmentation fault methods.
</p>
TicketarpitdmTue, 23 Aug 2016 18:49:02 GMTstatus changed
https://trac.sagemath.org/ticket/13215#comment:205
https://trac.sagemath.org/ticket/13215#comment:205
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
TickettscrimTue, 23 Aug 2016 19:54:00 GMTstatus changed
https://trac.sagemath.org/ticket/13215#comment:206
https://trac.sagemath.org/ticket/13215#comment:206
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<ul><li>You need to change the docstring of <code>_call</code> to reflect that its existence is only because of <code>@experimental</code>:
<pre class="wiki">Helper function for the ``__call__`` method to accommodate
the ``@experimental`` decorator.
</pre></li><li>I think you should make <code>operator_eval</code> into a <code>cpdef</code> method as it will likely be an important method (and should have a C version). You also need to change
<div class="wiki-code"><div class="code"><pre><span class="gd">-(<RingElement> c)*a
</span><span class="gi">+c * a
</span></pre></div></div>so the skew polynomial ring can support non-<code>RingElement</code> classes as a coefficients and to avoid an unneeded type check by Cython.
</li><li>You have added extra coercion checks for each step of things like <code>left_xgcd</code>. I feel that you would be better having a public method that does <code>@coerce_binop</code> and then private (<code>cdef</code>?) functions that assume both elements belong to the same parent. If you don't think this is so important, than don't feel compelled to change it.
</li><li>Are the <code>*_monic</code> methods useful to have at the public namespace (instead of (effectively hidden) <code>cdef</code> methods)? I feel one can just divide by the leading coefficient and that this operation will not be called frequently by a user.
</li></ul>
TicketjsrnTue, 23 Aug 2016 22:16:15 GMT
https://trac.sagemath.org/ticket/13215#comment:207
https://trac.sagemath.org/ticket/13215#comment:207
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:206" title="Comment 206">tscrim</a>:
</p>
<blockquote class="citation">
<ul><li>You have added extra coercion checks for each step of things like <code>left_xgcd</code>. I feel that you would be better having a public method that does <code>@coerce_binop</code> and then private (<code>cdef</code>?) functions that assume both elements belong to the same parent. If you don't think this is so important, than don't feel compelled to change it.
</li></ul></blockquote>
<p>
Seems like the gain would currently be very small, since these methods now mostly do not call each other internally. If this changes later on, the private method can be added at this time.
</p>
<blockquote class="citation">
<ul><li>Are the <code>*_monic</code> methods useful to have at the public namespace (instead of (effectively hidden) <code>cdef</code> methods)? I feel one can just divide by the leading coefficient and that this operation will not be called frequently by a user.
</li></ul></blockquote>
<p>
Usual commutative polynomials have a <code>monic</code> method. This is the analogue for non-commutative polynomials.
</p>
<p>
Best,
Johan
</p>
TickettscrimTue, 23 Aug 2016 22:22:47 GMT
https://trac.sagemath.org/ticket/13215#comment:208
https://trac.sagemath.org/ticket/13215#comment:208
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:207" title="Comment 207">jsrn</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:206" title="Comment 206">tscrim</a>:
</p>
<blockquote class="citation">
<ul><li>You have added extra coercion checks for each step of things like <code>left_xgcd</code>. I feel that you would be better having a public method that does <code>@coerce_binop</code> and then private (<code>cdef</code>?) functions that assume both elements belong to the same parent. If you don't think this is so important, than don't feel compelled to change it.
</li></ul></blockquote>
<p>
Seems like the gain would currently be very small, since these methods now mostly do not call each other internally. If this changes later on, the private method can be added at this time.
</p>
</blockquote>
<p>
However this feels it could be used in tight loops, and so while individual calls are marginal, the net sum over 10000 loops can add up. I leave it up to you.
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>Are the <code>*_monic</code> methods useful to have at the public namespace (instead of (effectively hidden) <code>cdef</code> methods)? I feel one can just divide by the leading coefficient and that this operation will not be called frequently by a user.
</li></ul></blockquote>
<p>
Usual commutative polynomials have a <code>monic</code> method. This is the analogue for non-commutative polynomials.
</p>
</blockquote>
<p>
Only for univariate, and in that case all it does is divide by the leading coefficient. However, in that case it extends the base ring so the leading coefficient is a unit, which is different than the behavior here. Also, I don't necessarily see the point of having <code>monic</code> in the public API for commutative polynomials (and don't take that as a guide for what must/should be included in the API).
</p>
TicketgitWed, 24 Aug 2016 05:48:38 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:209
https://trac.sagemath.org/ticket/13215#comment:209
<ul>
<li><strong>commit</strong>
changed from <em>5dcea53ec25b2cc19873230ce697ec3720d95238</em> to <em>8c283a85ca2c04b1598ce6efaa8768939a59283b</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=8c283a85ca2c04b1598ce6efaa8768939a59283b"><span class="icon"></span>8c283a8</a></td><td><code>made operator_eval a cpdef method. small fix to the operator_eval method.</code>
</td></tr></table>
TicketjsrnWed, 24 Aug 2016 12:59:13 GMT
https://trac.sagemath.org/ticket/13215#comment:210
https://trac.sagemath.org/ticket/13215#comment:210
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:208" title="Comment 208">tscrim</a>:
</p>
<blockquote class="citation">
<p>
However this feels it could be used in tight loops, and so while individual calls are marginal, the net sum over 10000 loops can add up. I leave it up to you.
</p>
</blockquote>
<p>
It's mostly the relative gain that's important, which is still low as long as the functions don't call each other a lot.
</p>
<blockquote class="citation">
<p>
Only for univariate, and in that case all it does is divide by the leading coefficient. However, in that case it extends the base ring so the leading coefficient is a unit, which is different than the behavior here. Also, I don't necessarily see the point of having <code>monic</code> in the public API for commutative polynomials (and don't take that as a guide for what must/should be included in the API).
</p>
</blockquote>
<p>
I think it's a handy helper function that I think people will expect. I've used <code>monic</code> for univariate polynomials many times. (for multivariate polynomials, it's much more ambiguous what can be meant so the helper function is less useful).
</p>
TicketcarusoWed, 24 Aug 2016 13:05:49 GMT
https://trac.sagemath.org/ticket/13215#comment:211
https://trac.sagemath.org/ticket/13215#comment:211
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:208" title="Comment 208">tscrim</a>:
</p>
<blockquote class="citation">
<p>
However this feels it could be used in tight loops, and so while individual calls are marginal, the net sum over 10000 loops can add up. I leave it up to you.
</p>
</blockquote>
<p>
Maybe, we can leave as it currently is for this ticket and come back to this in ticket <a class="closed ticket" href="https://trac.sagemath.org/ticket/21088" title="enhancement: Base Class for Skew Polynomials over Finite Fields (closed: wontfix)">#21088</a> where inplace fast functions are designed.
</p>
TicketarpitdmWed, 24 Aug 2016 13:06:20 GMT
https://trac.sagemath.org/ticket/13215#comment:212
https://trac.sagemath.org/ticket/13215#comment:212
<p>
I've made the <code>operator_eval</code> method a cpdef method. And I've edited the docstring to mention that it is a helper function to accommodate the <code>@experimental</code>decorator.
</p>
<p>
Best,
Arpit.
</p>
TicketarpitdmWed, 24 Aug 2016 13:06:40 GMTstatus changed
https://trac.sagemath.org/ticket/13215#comment:213
https://trac.sagemath.org/ticket/13215#comment:213
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
TickettscrimWed, 24 Aug 2016 17:25:18 GMT
https://trac.sagemath.org/ticket/13215#comment:214
https://trac.sagemath.org/ticket/13215#comment:214
<p>
I am okay with the current state. Any other objections?
</p>
TicketgitWed, 24 Aug 2016 22:43:15 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:215
https://trac.sagemath.org/ticket/13215#comment:215
<ul>
<li><strong>commit</strong>
changed from <em>8c283a85ca2c04b1598ce6efaa8768939a59283b</em> to <em>1f441357f62c17e394431028166f01adc3012c37</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=b9d1ecdc45ed89cfc75243b41ae3f98c414b71fd"><span class="icon"></span>b9d1ecd</a></td><td><code>Fixed some of the doctests I just broke</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=6efcd02b562488013984086897a2c98a2c3af9fa"><span class="icon"></span>6efcd02</a></td><td><code>more doc</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=8a991b7bfa569a9eacbca32c8665dcc804b114e2"><span class="icon"></span>8a991b7</a></td><td><code>more doc</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=69e33989deba991691898108fc2bf99a09d43553"><span class="icon"></span>69e3398</a></td><td><code>Evaluation outside the base ring is *not* possible</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=472193b67fa35068ddde6067ee95e0ed0a3612c6"><span class="icon"></span>472193b</a></td><td><code>improve __call__ doc</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=430020496b5c336b48d6bbcd5a36cf973078d2b7"><span class="icon"></span>4300204</a></td><td><code>More various doc</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=36cc78817e931c4bef7206bd926decf415d2f6e1"><span class="icon"></span>36cc788</a></td><td><code>Rephrase the same phrase repeated many times</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=dd8b275246da50151a6b6f780f8e744c859b9e73"><span class="icon"></span>dd8b275</a></td><td><code>More doc string fixes</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=47ba6f0799993d7fd98e1a559fcb9dab32e9574d"><span class="icon"></span>47ba6f0</a></td><td><code>There should not be a mod, but rather left_mod and right_mod</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=1f441357f62c17e394431028166f01adc3012c37"><span class="icon"></span>1f44135</a></td><td><code>Doc fixes for the last methods</code>
</td></tr></table>
TicketjsrnWed, 24 Aug 2016 22:45:01 GMT
https://trac.sagemath.org/ticket/13215#comment:216
https://trac.sagemath.org/ticket/13215#comment:216
<p>
I went through all docs a last time, polishing many docs and also modifying a few tests and even an implementation or two. Please review.
</p>
<p>
Best,
Johan
</p>
TickettscrimWed, 24 Aug 2016 23:03:37 GMT
https://trac.sagemath.org/ticket/13215#comment:217
https://trac.sagemath.org/ticket/13215#comment:217
<p>
I disagree with the following:
</p>
<ul><li>This is probably not true:
<pre class="wiki"> - ``monic`` -- boolean (default: ``True``). Return whether the right gcd
should be normalized to be monic. This parameter must be passed as a
named argument.
</pre>At least I don't see a reason why must go <code>monic=True</code> as an argument (for everything but <code>left_xgcd</code>).
Actually, I don't see why <code>left_xgcd</code> has unlimited number of arguments but <code>right_xgcd</code> does not.
</li><li>This change make it seem like there is one unique skew polynomial ring (to rule them all :P).
<div class="wiki-code"><div class="code"><pre><span class="gd">- Return the globally unique skew polynomial ring with given
- properties and variable name or names.
</span><span class="gi">+ Return the globally unique skew polynomial ring.
</span></pre></div></div></li><li>The part about the general case not being implemented should be in a <code>.. NOTE::</code> or <code>.. WARNING::</code> block:
<div class="wiki-code"><div class="code"><pre> r"""
Return ``True`` if this skew polynomial is a unit.
<span class="gd">- .. NOTE::
</span><span class="gi">+ When the base ring `R` is an integral domain, then a skew polynomial `f`
+ is a unit if and only if degree of `f` is `0` and `f` is then a unit in
+ `R`.
</span>
<span class="gd">- When the base ring `R` is an integral domain, then a skew
- polynomial `f` is a unit if and only if degree of `f` is
- `0` and `f` is then a unit in `R`. The general case is not
- yet implemented.
</span><span class="gi">+ The case when `R` is not an integral domain is not yet implemented.
</span>
EXAMPLES::
</pre></div></div></li><li>You should revert this change so we keep it under 80 characters/line:
<div class="wiki-code"><div class="code"><pre> else:
return False
else:
<span class="gd">- raise NotImplementedError("support for determining if skew polynomial"
- " is a unit is not available when the base"
- " ring of the parent skew polynomial"
- " ring is not an integral domain")
</span><span class="gi">+ raise NotImplementedError("is_unit is not implemented for skew polynomial rings over base rings which are not integral domains.")
</span>
def is_nilpotent(self):
r"""
</pre></div></div></li><li>You should not break <code>:trac:`13215`</code> across multiple lines (I might just be paranoid for this one though).
</li><li>AFAIK, only operator evaluation is implemented, so I don't understand this removal:
<div class="wiki-code"><div class="code"><pre> .. NOTE::
<span class="gd">- Currently, only "operator evaluation" of skew polynomials is implemented
- (see :meth:`.operator_eval`).
</span> There are two other common notions of evaluation of `f(x)` at some element `a`
from the base ring, namely the remainder after left or right modulo by `x-a`.
</pre></div></div></li><li><code>`__call__`</code> is not latex, it should be <code>:meth:`__call__`</code> (or <code>``__call__``</code>).
</li></ul><p>
Otherwise all your changes look good to me.
</p>
TicketgitThu, 25 Aug 2016 07:47:07 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:218
https://trac.sagemath.org/ticket/13215#comment:218
<ul>
<li><strong>commit</strong>
changed from <em>1f441357f62c17e394431028166f01adc3012c37</em> to <em>ef7fbe02c45e876bf17e54e4467db815ca0be428</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=8ae28e0f88c093bb1f853fb8ecedc57e71f7260f"><span class="icon"></span>8ae28e0</a></td><td><code>monic can be passed as unnamed. And rm *-args from left_xgcd</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=ef7fbe02c45e876bf17e54e4467db815ca0be428"><span class="icon"></span>ef7fbe0</a></td><td><code>Fix reviewer's other comments</code>
</td></tr></table>
TicketjsrnThu, 25 Aug 2016 07:54:47 GMT
https://trac.sagemath.org/ticket/13215#comment:219
https://trac.sagemath.org/ticket/13215#comment:219
<p>
Thanks for carefully going through all the modifications.
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:217" title="Comment 217">tscrim</a>:
</p>
<blockquote class="citation">
<ul><li>This is probably not true:
<pre class="wiki"> - ``monic`` -- boolean (default: ``True``). Return whether the right gcd
should be normalized to be monic. This parameter must be passed as a
named argument.
</pre>At least I don't see a reason why must go <code>monic=True</code> as an argument (for everything but <code>left_xgcd</code>).
Actually, I don't see why <code>left_xgcd</code> has unlimited number of arguments but <code>right_xgcd</code> does not.
</li></ul></blockquote>
<p>
Indeed. I've chatted with Xavier, and the original reason for the "This parameter must be passed as named argument" (and perhaps the <code>*</code> in <code>left_xgcd</code>) is the broken behaviour of <code>coerce_binop</code>. This has now been fixed, see <a class="closed ticket" href="https://trac.sagemath.org/ticket/21322" title="defect: coerce_binop has dangerous behaviour wrt additional arguments (closed: fixed)">#21322</a>. Therefore I've now set the doc and the code to what it should be as soon as <a class="closed ticket" href="https://trac.sagemath.org/ticket/21322" title="defect: coerce_binop has dangerous behaviour wrt additional arguments (closed: fixed)">#21322</a> is merged.
</p>
<blockquote class="citation">
<ul><li>This change make it seem like there is one unique skew polynomial ring (to rule them all :P).
</li></ul></blockquote>
<p>
OK, that change was a bit fast ;-)
</p>
<blockquote class="citation">
<ul><li>The part about the general case not being implemented should be in a <code>.. NOTE::</code> or <code>.. WARNING::</code> block:
</li></ul></blockquote>
<p>
I found this overkill, which is why I removed it. But if you insist.
</p>
<blockquote class="citation">
<ul><li>You should revert this change so we keep it under 80 characters/line:
</li></ul></blockquote>
<p>
I disagree. It makes the source code look terrible. Also:
</p>
<pre class="wiki">$ grep '.\{80,\}' -r src/sage | grep "raise" | wc -l
4703
</pre><p>
So the 80 chars/line is defacto not a convention upheld in raise-statements.
</p>
<blockquote class="citation">
<ul><li>You should not break <code>:trac:`13215`</code> across multiple lines (I might just be paranoid for this one though).
</li></ul></blockquote>
<p>
Indeed. Emacs reflow doesn't understand Sphinx magic syntax probably doesn't allow line breaks :-)
</p>
<blockquote class="citation">
<ul><li>AFAIK, only operator evaluation is implemented, so I don't understand this removal:
<div class="wiki-code"><div class="code"><pre> .. NOTE::
<span class="gd">- Currently, only "operator evaluation" of skew polynomials is implemented
- (see :meth:`.operator_eval`).
</span> There are two other common notions of evaluation of `f(x)` at some element `a`
from the base ring, namely the remainder after left or right modulo by `x-a`.
</pre></div></div></li></ul></blockquote>
<p>
The entirety of the <code>__call__</code> doc reads better now, IMHO: the first line clearly states that operator evaluation is considered. So the note doesn't need to repeat this.
</p>
<blockquote class="citation">
<ul><li><code>`__call__`</code> is not latex, it should be <code>:meth:`__call__`</code> (or <code>``__call__``</code>).
</li></ul></blockquote>
<p>
Indeed.
</p>
<blockquote class="citation">
<p>
Otherwise all your changes look good to me.
</p>
</blockquote>
<p>
Great!
</p>
TicketcarusoThu, 25 Aug 2016 12:32:00 GMT
https://trac.sagemath.org/ticket/13215#comment:220
https://trac.sagemath.org/ticket/13215#comment:220
<p>
Another remark: Should we define the methods <code>left_quo</code> and <code>right_quo</code> as well? And create aliases <code>left_rem</code> and <code>right_rem</code> for <code>left_mod</code> and <code>right_mod</code> respectively?
</p>
TickettscrimThu, 25 Aug 2016 14:16:59 GMT
https://trac.sagemath.org/ticket/13215#comment:221
https://trac.sagemath.org/ticket/13215#comment:221
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:219" title="Comment 219">jsrn</a>:
</p>
<blockquote class="citation">
<p>
Thanks for carefully going through all the modifications.
</p>
</blockquote>
<p>
Thank you for your work on this (and dealing with my griping).
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:217" title="Comment 217">tscrim</a>:
</p>
<blockquote class="citation">
<ul><li>This is probably not true:
<pre class="wiki"> - ``monic`` -- boolean (default: ``True``). Return whether the right gcd
should be normalized to be monic. This parameter must be passed as a
named argument.
</pre>At least I don't see a reason why must go <code>monic=True</code> as an argument (for everything but <code>left_xgcd</code>).
Actually, I don't see why <code>left_xgcd</code> has unlimited number of arguments but <code>right_xgcd</code> does not.
</li></ul></blockquote>
<p>
Indeed. I've chatted with Xavier, and the original reason for the "This parameter must be passed as named argument" (and perhaps the <code>*</code> in <code>left_xgcd</code>) is the broken behaviour of <code>coerce_binop</code>. This has now been fixed, see <a class="closed ticket" href="https://trac.sagemath.org/ticket/21322" title="defect: coerce_binop has dangerous behaviour wrt additional arguments (closed: fixed)">#21322</a>. Therefore I've now set the doc and the code to what it should be as soon as <a class="closed ticket" href="https://trac.sagemath.org/ticket/21322" title="defect: coerce_binop has dangerous behaviour wrt additional arguments (closed: fixed)">#21322</a> is merged.
</p>
</blockquote>
<p>
Does <a class="closed ticket" href="https://trac.sagemath.org/ticket/21322" title="defect: coerce_binop has dangerous behaviour wrt additional arguments (closed: fixed)">#21322</a> need to be a dependency of this ticket then?
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>You should revert this change so we keep it under 80 characters/line:
</li></ul></blockquote>
<p>
I disagree. It makes the source code look terrible. Also:
</p>
<pre class="wiki">$ grep '.\{80,\}' -r src/sage | grep "raise" | wc -l
4703
</pre><p>
So the 80 chars/line is defacto not a convention upheld in raise-statements.
</p>
</blockquote>
<p>
It makes the source code look fine if you are using < ~160 chars wide editors. We do try to stick to ~80 character lines overall in the code, but it is not absolute. Plus, there is a big difference between 90 chars and ~160 chars. IMO, a more fair comparison would be to look at how many <code>raise</code> statements are 80,90,100,100+ lines. I do not see the gain for the long line unless you have an editor with a very large character line limit (literally).
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>AFAIK, only operator evaluation is implemented, so I don't understand this removal:
<div class="wiki-code"><div class="code"><pre> .. NOTE::
<span class="gd">- Currently, only "operator evaluation" of skew polynomials is implemented
- (see :meth:`.operator_eval`).
</span> There are two other common notions of evaluation of `f(x)` at some element `a`
from the base ring, namely the remainder after left or right modulo by `x-a`.
</pre></div></div></li></ul></blockquote>
<p>
The entirety of the <code>__call__</code> doc reads better now, IMHO: the first line clearly states that operator evaluation is considered. So the note doesn't need to repeat this.
</p>
</blockquote>
<p>
The key word is "only", and the <code>__call__</code> doc does not mention this (nor is it anywhere else IIRC).
</p>
TicketgitThu, 25 Aug 2016 14:22:24 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:222
https://trac.sagemath.org/ticket/13215#comment:222
<ul>
<li><strong>commit</strong>
changed from <em>ef7fbe02c45e876bf17e54e4467db815ca0be428</em> to <em>81b8fb8c2b1f4430388e27e3028422caeb28d0a9</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=a649c67b473c49a10266241dfac31ced3af27331"><span class="icon"></span>a649c67</a></td><td><code>fixes to the documentation</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=f54e80e81f4f658f50950298d677f4a9d076ffeb"><span class="icon"></span>f54e80e</a></td><td><code>removed method reverse</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=1336c57401aa69c84abac82a01596db4f7f42de7"><span class="icon"></span>1336c57</a></td><td><code>small fixes to the documentation</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=81b8fb8c2b1f4430388e27e3028422caeb28d0a9"><span class="icon"></span>81b8fb8</a></td><td><code>small fixes to the ring_constructor file</code>
</td></tr></table>
TicketarpitdmThu, 25 Aug 2016 14:28:02 GMT
https://trac.sagemath.org/ticket/13215#comment:223
https://trac.sagemath.org/ticket/13215#comment:223
<p>
I went through the docs once and clarified them in some places where it was not clear/missing and made some small changes to standardize the docs whereever possible.
</p>
<p>
Based on discussions with @caruso and @jsrn, the <code>def reverse</code> method is inappropriate in that, reversing a skew polynomial actually involves creating a new skew polynomial ring over the inverse twist map. And that requires more consideration since twist map cannot always be inverted in Sage and also perhaps since "dual_ring" or something like that could also be nice to have.
</p>
<p>
Best,
Arpit.
</p>
TicketdarijThu, 25 Aug 2016 14:30:03 GMTcc changed
https://trac.sagemath.org/ticket/13215#comment:224
https://trac.sagemath.org/ticket/13215#comment:224
<ul>
<li><strong>cc</strong>
<em>darij</em> removed
</li>
</ul>
TickettscrimThu, 25 Aug 2016 14:35:08 GMT
https://trac.sagemath.org/ticket/13215#comment:225
https://trac.sagemath.org/ticket/13215#comment:225
<p>
Changes like:
</p>
<div class="wiki-code"><div class="code"><pre><span class="gu">@@ -134,7 +134,7 @@ cdef class SkewPolynomial(AlgebraElement):
</span> where `\sigma` is the base ring automorphism. This is called
the *operator evaluation* method.
<span class="gd">- EXAMPLES:
</span><span class="gi">+ EXAMPLES::
</span>
We illustrate some functionalities implemented in this class.
</pre></div></div><p>
are wrong because text follows, not (indented) code. So the last commit should be reverted. Also, I disagree with changing the <code>.. TODO::</code> block because it is a TODO.
</p>
<p>
One other thing I just noticed, a better test than <code>if not isinstance(base_ring, ring.CommutativeRing):</code> would be <code>if base_ring not in Rings().Commutative():</code> as it is a mathematical check, not an implementation check.
</p>
<p>
It is also okay to have methods that rely on others than can error out in certain cases when implementations are not done yet. Just FYI.
</p>
<p>
You also did not need to change <code>r"""</code> for every docstring (it doesn't hurt, but it is only necessary when you have escape <code>\</code> characters).
</p>
TicketgitThu, 25 Aug 2016 14:55:49 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:226
https://trac.sagemath.org/ticket/13215#comment:226
<ul>
<li><strong>commit</strong>
changed from <em>81b8fb8c2b1f4430388e27e3028422caeb28d0a9</em> to <em>b6e77d2507948d131b48224ef3c217a79f79bb51</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=b6e77d2507948d131b48224ef3c217a79f79bb51"><span class="icon"></span>b6e77d2</a></td><td><code>fixed html documentation build errors</code>
</td></tr></table>
TicketarpitdmThu, 25 Aug 2016 15:05:51 GMT
https://trac.sagemath.org/ticket/13215#comment:227
https://trac.sagemath.org/ticket/13215#comment:227
<blockquote class="citation">
<ul><li> EXAMPLES:
</li></ul><p>
+ EXAMPLES::
}}}
are wrong because text follows, not (indented) code. So the last commit should be reverted.
</p>
</blockquote>
<p>
I just checked whether the html documentation builds and in this particular case, it did not give an error while in a couple of other places it did.
</p>
<blockquote class="citation">
<p>
Also, I disagree with changing the <code>.. TODO::</code> block because it is a TODO.
</p>
</blockquote>
<p>
Do you mean, changing the name from <code>TODO</code> to <code>NOTE</code> or the description as well?
</p>
<blockquote class="citation">
<p>
One other thing I just noticed, a better test than <code>if not isinstance(base_ring, ring.CommutativeRing):</code> would be <code>if base_ring not in Rings().Commutative():</code> as it is a mathematical check, not an implementation check.
</p>
</blockquote>
<p>
Okay. I will change that.
</p>
<blockquote class="citation">
<p>
It is also okay to have methods that rely on others than can error out in certain cases when implementations are not done yet. Just FYI.
</p>
<p>
You also did not need to change <code>r"""</code> for every docstring (it doesn't hurt, but it is only necessary when you have escape <code>\</code> characters).
</p>
</blockquote>
<p>
I was just trying to follow the conventions in other files where I saw that <code>r"""</code> is used everywhere. So while, I was going through the code, I figured, it didn't hurt to change it.
</p>
TickettscrimThu, 25 Aug 2016 15:12:41 GMT
https://trac.sagemath.org/ticket/13215#comment:228
https://trac.sagemath.org/ticket/13215#comment:228
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:227" title="Comment 227">arpitdm</a>:
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
Also, I disagree with changing the <code>.. TODO::</code> block because it is a TODO.
</p>
</blockquote>
<p>
Do you mean, changing the name from <code>TODO</code> to <code>NOTE</code> or the description as well?
</p>
</blockquote>
<p>
You made a change <code>.. TODO::</code> -> <code>.. NOTE::</code>, where the message is describing a TODO.
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
You also did not need to change <code>r"""</code> for every docstring (it doesn't hurt, but it is only necessary when you have escape <code>\</code> characters).
</p>
</blockquote>
<p>
I was just trying to follow the conventions in other files where I saw that <code>r"""</code> is used everywhere. So while, I was going through the code, I figured, it didn't hurt to change it.
</p>
</blockquote>
<p>
Yes, it's perfectly fine (in some aspects, it should be the standard way we do docstrings in Sage). This was another just FYI and future reference.
</p>
TicketgitThu, 25 Aug 2016 15:19:11 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:229
https://trac.sagemath.org/ticket/13215#comment:229
<ul>
<li><strong>commit</strong>
changed from <em>b6e77d2507948d131b48224ef3c217a79f79bb51</em> to <em>97ba596ec01b04bc06190bfe7e544f608be03545</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=27c05c62584da45153b7e7e113a5adeecd88317f"><span class="icon"></span>27c05c6</a></td><td><code>improved check on commutativity of base ring</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=97ba596ec01b04bc06190bfe7e544f608be03545"><span class="icon"></span>97ba596</a></td><td><code>reverted NOTE back to TODO</code>
</td></tr></table>
TicketgitThu, 25 Aug 2016 21:07:23 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:230
https://trac.sagemath.org/ticket/13215#comment:230
<ul>
<li><strong>commit</strong>
changed from <em>97ba596ec01b04bc06190bfe7e544f608be03545</em> to <em>35fcdf24bfd2824a59c9053662ea5f133e94363e</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=92564c5f71f533e9b8108a26d4d5d74152bbde8a"><span class="icon"></span>92564c5</a></td><td><code>rewrap</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=35fcdf24bfd2824a59c9053662ea5f133e94363e"><span class="icon"></span>35fcdf2</a></td><td><code>Merge branch 'public/rings/skew_polynomials-13215' of trac.sagemath.org:sage into 13215_skew_polynomials</code>
</td></tr></table>
TicketjsrnThu, 25 Aug 2016 21:30:26 GMT
https://trac.sagemath.org/ticket/13215#comment:231
https://trac.sagemath.org/ticket/13215#comment:231
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:221" title="Comment 221">tscrim</a>:
</p>
<blockquote class="citation">
<p>
Does <a class="closed ticket" href="https://trac.sagemath.org/ticket/21322" title="defect: coerce_binop has dangerous behaviour wrt additional arguments (closed: fixed)">#21322</a> need to be a dependency of this ticket then?
</p>
</blockquote>
<p>
Not in our opinion: what we do here is the intended behaviour of coerce_binop,
and the decorated methods works as well as all other methods in Sage decorated
with coerce_binop. So <a class="closed ticket" href="https://trac.sagemath.org/ticket/21322" title="defect: coerce_binop has dangerous behaviour wrt additional arguments (closed: fixed)">#21322</a> is just a (already known) bug that should be fixed.
</p>
<blockquote class="citation">
<p>
It makes the source code look fine if you are using < ~160 chars wide editors.
</p>
</blockquote>
<p>
??? The line is 141 chars. Also, having a < 160 chars editor doesn't make the
old 4-line wrapping look good.
</p>
<blockquote class="citation">
<p>
We do try to stick to ~80 character lines overall in the code, but it is not
</p>
</blockquote>
<p>
absolute.
</p>
<p>
Indeed my point before.
</p>
<blockquote class="citation">
<p>
Plus, there is a big difference between 90 chars and ~160 chars. IMO, a more
fair comparison would be to look at how many <code>raise</code> statements are
80,90,100,100+ lines. I do not see the gain for the long line unless you have an
editor with a very large character line limit (literally).
</p>
</blockquote>
<pre class="wiki"> $ grep '.\{100,\}' -r src/sage | grep "raise" | wc -l
1628
$ grep '.\{120,\}' -r src/sage | grep "raise" | wc -l
588
$ grep '.\{140,\}' -r src/sage | grep "raise" | wc -l
210
</pre><p>
Also, btw:
</p>
<pre class="wiki"> $ grep '.\{140,\}' -r src/sage | wc -l
10459
</pre><p>
So it's not like you're not likely to come across a long line in your editor
while editing Sage codes.
</p>
<blockquote class="citation">
<blockquote class="citation">
<blockquote class="citation">
<ul><li>AFAIK, only operator evaluation is implemented, so I don't understand this removal:
<div class="wiki-code"><div class="code"><pre> .. NOTE::
<span class="gd">- Currently, only "operator evaluation" of skew polynomials is implemented
- (see :meth:`.operator_eval`).
</span> There are two other common notions of evaluation of `f(x)` at some element `a`
from the base ring, namely the remainder after left or right modulo by `x-a`.
</pre></div></div></li></ul></blockquote>
<p>
The entirety of the <code>__call__</code> doc reads better now, IMHO: the first line clearly states that operator evaluation is considered. So the note doesn't need to repeat this.
</p>
</blockquote>
<p>
The key word is "only", and the <code>__call__</code> doc does not mention this (nor is it anywhere else IIRC).
</p>
</blockquote>
<p>
It says "The current calling convention might change in the future to
accommodate these. Therefore, the current method has been marked as
experimental.". Try to read the doc-string in its entirety -- it really does say
everything.
</p>
<p>
Best,
Johan
</p>
TicketjsrnThu, 25 Aug 2016 21:32:02 GMT
https://trac.sagemath.org/ticket/13215#comment:232
https://trac.sagemath.org/ticket/13215#comment:232
<p>
Oh yeah, forgot to add about the whole wrapping thing: I now pushed a compromise where the line is wrapped in two but takes up 92 chars.
</p>
TicketjsrnThu, 25 Aug 2016 21:32:33 GMTkeywords changed
https://trac.sagemath.org/ticket/13215#comment:233
https://trac.sagemath.org/ticket/13215#comment:233
<ul>
<li><strong>keywords</strong>
<em>sd75</em> added
</li>
</ul>
TicketarpitdmFri, 26 Aug 2016 11:47:19 GMT
https://trac.sagemath.org/ticket/13215#comment:234
https://trac.sagemath.org/ticket/13215#comment:234
<p>
This ticket is still open for review. Are there any further objections?
</p>
<p>
-Arpit.
</p>
TickettscrimFri, 26 Aug 2016 14:59:47 GMT
https://trac.sagemath.org/ticket/13215#comment:235
https://trac.sagemath.org/ticket/13215#comment:235
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:231" title="Comment 231">jsrn</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:221" title="Comment 221">tscrim</a>:
</p>
<blockquote class="citation">
<p>
It makes the source code look fine if you are using < ~160 chars wide editors.
</p>
</blockquote>
<p>
??? The line is 141 chars. Also, having a < 160 chars editor doesn't make the
old 4-line wrapping look good.
</p>
</blockquote>
<p>
It makes it more consistent and it is a standard way to split long lines of text by PEP8.
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
We do try to stick to ~80 character lines overall in the code, but it is not
</p>
</blockquote>
<p>
absolute.
</p>
<p>
Indeed my point before.
</p>
</blockquote>
<p>
I can accept the 92 char lines as a compromise.
</p>
<blockquote class="citation">
<p>
Also, btw:
</p>
<pre class="wiki"> $ grep '.\{140,\}' -r src/sage | wc -l
10459
</pre><p>
So it's not like you're not likely to come across a long line in your editor
while editing Sage codes.
</p>
</blockquote>
<p>
There are a number of lines that are forced to be long because they are large numbers with no spaces. Plus, I have no idea exactly how many lines of code we have in Sage, but this smells of a fallacy.
</p>
<blockquote class="citation">
<blockquote class="citation">
<blockquote class="citation">
<blockquote class="citation">
<ul><li>AFAIK, only operator evaluation is implemented, so I don't understand this removal:
<div class="wiki-code"><div class="code"><pre> .. NOTE::
<span class="gd">- Currently, only "operator evaluation" of skew polynomials is implemented
- (see :meth:`.operator_eval`).
</span> There are two other common notions of evaluation of `f(x)` at some element `a`
from the base ring, namely the remainder after left or right modulo by `x-a`.
</pre></div></div></li></ul></blockquote>
<p>
The entirety of the <code>__call__</code> doc reads better now, IMHO: the first line clearly states that operator evaluation is considered. So the note doesn't need to repeat this.
</p>
</blockquote>
<p>
The key word is "only", and the <code>__call__</code> doc does not mention this (nor is it anywhere else IIRC).
</p>
</blockquote>
<p>
It says "The current calling convention might change in the future to
accommodate these. Therefore, the current method has been marked as
experimental.". Try to read the doc-string in its entirety -- it really does say
everything.
</p>
</blockquote>
<p>
I very carefully had read it, and no, it does not say everything. It does not say that this is the <em>only</em> type of evaluation currently implemented. I know this is dangerously close to bikeshedding because this is only visible at the code level. Yet, I think it is important to state why we are (currently) doing things this way: there currently is no other option.
</p>
TicketgitSat, 27 Aug 2016 06:47:45 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:236
https://trac.sagemath.org/ticket/13215#comment:236
<ul>
<li><strong>commit</strong>
changed from <em>35fcdf24bfd2824a59c9053662ea5f133e94363e</em> to <em>e72a8cc6627f68c38c2ae84ccd52bfe9c9e10fd5</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=e72a8cc6627f68c38c2ae84ccd52bfe9c9e10fd5"><span class="icon"></span>e72a8cc</a></td><td><code>reinstate as per reviewer's insistence</code>
</td></tr></table>
TicketjsrnSat, 27 Aug 2016 06:48:39 GMT
https://trac.sagemath.org/ticket/13215#comment:237
https://trac.sagemath.org/ticket/13215#comment:237
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:235" title="Comment 235">tscrim</a>:
</p>
<blockquote class="citation">
<p>
I very carefully had read it, and no, it does not say everything. It does not say that this is the <em>only</em> type of evaluation currently implemented. I know this is dangerously close to bikeshedding because this is only visible at the code level. Yet, I think it is important to state why we are (currently) doing things this way: there currently is no other option.
</p>
</blockquote>
<p>
Bikeshedding indeed...
</p>
TicketjsrnSat, 27 Aug 2016 06:59:24 GMT
https://trac.sagemath.org/ticket/13215#comment:238
https://trac.sagemath.org/ticket/13215#comment:238
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13215#comment:237" title="Comment 237">jsrn</a>:
</p>
<blockquote class="citation">
<p>
Bikeshedding indeed...
</p>
</blockquote>
<p>
Sorry for this grumbling. I shouldn't reply to comments before my morning coffee.
</p>
<p>
Anyway, it seems the issues are getting exponentially less important, so perhaps we can positive review this ticket soon.
Best,
Johan
</p>
TickettscrimSat, 27 Aug 2016 16:14:33 GMTstatus changed
https://trac.sagemath.org/ticket/13215#comment:239
https://trac.sagemath.org/ticket/13215#comment:239
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
Sorry I have a bit of a stick up my @$* about this, but I'd like to get this right. So we don't have to deal with issues because someone came along and changed things due to a lack of documentation.
</p>
<p>
Anyways, I believe we are at a positive review.
</p>
TicketvbraunSun, 28 Aug 2016 08:09:09 GMTstatus changed
https://trac.sagemath.org/ticket/13215#comment:240
https://trac.sagemath.org/ticket/13215#comment:240
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
PDF docs fail
</p>
<pre class="wiki">[docpdf] LaTeX Warning: Hyper reference `sage/rings/polynomial/skew_polynomial_element:s
[docpdf] age.rings.polynomial.skew_polynomial_element.SkewPolynomialBaseringInjection' o
[docpdf] n page 471 undefined on input line 39011.
[docpdf]
[docpdf] ! Missing { inserted.
[docpdf] <to be read again>
[docpdf] _
[docpdf] l.39015 The current semantics of \(__
[docpdf] call__\) are experimental, so a warning...
[docpdf]
[docpdf] ?
[docpdf] ! Emergency stop.
[docpdf] <to be read again>
[docpdf] _
[docpdf] l.39015 The current semantics of \(__
[docpdf] call__\) are experimental, so a warning...
[docpdf]
[docpdf] ! ==> Fatal error occurred, no output PDF file produced!
[docpdf] Transcript written on polynomial_rings.log.
</pre>
TicketgitMon, 29 Aug 2016 13:28:26 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:241
https://trac.sagemath.org/ticket/13215#comment:241
<ul>
<li><strong>commit</strong>
changed from <em>e72a8cc6627f68c38c2ae84ccd52bfe9c9e10fd5</em> to <em>204be6593bf4ec184c87292f96eeba22c3038087</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=204be6593bf4ec184c87292f96eeba22c3038087"><span class="icon"></span>204be65</a></td><td><code>fix doc building error</code>
</td></tr></table>
TickettscrimMon, 29 Aug 2016 13:57:37 GMTstatus changed
https://trac.sagemath.org/ticket/13215#comment:242
https://trac.sagemath.org/ticket/13215#comment:242
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>positive_review</em>
</li>
</ul>
<p>
I'm assuming that you wanted this back to needs review.
</p>
TicketjsrnMon, 29 Aug 2016 14:14:20 GMT
https://trac.sagemath.org/ticket/13215#comment:243
https://trac.sagemath.org/ticket/13215#comment:243
<p>
Indeed, thanks. I seem to have too many things on my stack today ;-)
</p>
TickettscrimMon, 29 Aug 2016 14:30:05 GMT
https://trac.sagemath.org/ticket/13215#comment:244
https://trac.sagemath.org/ticket/13215#comment:244
<p>
This is now one less. :)
</p>
TicketvbraunMon, 29 Aug 2016 22:27:23 GMTstatus changed
https://trac.sagemath.org/ticket/13215#comment:245
https://trac.sagemath.org/ticket/13215#comment:245
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Fails on 32-bit
</p>
<pre class="wiki">sage -t --long src/sage/rings/polynomial/skew_polynomial_element.pyx
**********************************************************************
File "src/sage/rings/polynomial/skew_polynomial_element.pyx", line 299, in sage.rings.polynomial.skew_polynomial_element.SkewPolynomial.__hash__
Failed example:
h = hash(a); h
Expected:
-1717348446110052408
Got:
-368406584
**********************************************************************
1 item had failures:
1 of 6 in sage.rings.polynomial.skew_polynomial_element.SkewPolynomial.__hash__
[722 tests, 1 failure, 0.57 s]
</pre>
TickettscrimTue, 30 Aug 2016 04:41:03 GMT
https://trac.sagemath.org/ticket/13215#comment:246
https://trac.sagemath.org/ticket/13215#comment:246
<p>
Ah, right 32v64 bit. @jsm @arpitdm I would just make the doctest <code>hash(a) == hash(a)</code> and you can set a positive review on my behalf once that is done.
</p>
TicketgitTue, 30 Aug 2016 07:05:54 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:247
https://trac.sagemath.org/ticket/13215#comment:247
<ul>
<li><strong>commit</strong>
changed from <em>204be6593bf4ec184c87292f96eeba22c3038087</em> to <em>ddb5d7fddfa598de92c5935ea7aeed7c309f03ad</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=ddb5d7fddfa598de92c5935ea7aeed7c309f03ad"><span class="icon"></span>ddb5d7f</a></td><td><code>Merge branch 'public/rings/skew_polynomials-13215' of trac.sagemath.org:sage into 13215_skew_polynomials</code>
</td></tr></table>
TicketgitTue, 30 Aug 2016 07:07:05 GMTcommit changed
https://trac.sagemath.org/ticket/13215#comment:248
https://trac.sagemath.org/ticket/13215#comment:248
<ul>
<li><strong>commit</strong>
changed from <em>ddb5d7fddfa598de92c5935ea7aeed7c309f03ad</em> to <em>7cb0ccebabc3eeabab0e3765941ad4a1b79cab1c</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=7cb0ccebabc3eeabab0e3765941ad4a1b79cab1c"><span class="icon"></span>7cb0cce</a></td><td><code>hash doctest as per reviewer's suggestion</code>
</td></tr></table>
TicketjsrnTue, 30 Aug 2016 07:07:46 GMTstatus changed
https://trac.sagemath.org/ticket/13215#comment:249
https://trac.sagemath.org/ticket/13215#comment:249
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>positive_review</em>
</li>
</ul>
<p>
Done. Let's try again :-)
</p>
TicketvbraunTue, 30 Aug 2016 22:20:25 GMTstatus, branch changed; resolution set
https://trac.sagemath.org/ticket/13215#comment:250
https://trac.sagemath.org/ticket/13215#comment:250
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>branch</strong>
changed from <em>public/rings/skew_polynomials-13215</em> to <em>7cb0ccebabc3eeabab0e3765941ad4a1b79cab1c</em>
</li>
</ul>
Ticket