Sage: Ticket #8800: Doctest coverage of categories - numerous coercion fixes
https://trac.sagemath.org/ticket/8800
<p>
According to William, the doctest coverage of categories is too low:
</p>
<pre class="wiki">action.pyx: 0% (0 of 31)
functor.pyx: 17% (3 of 17)
map.pyx: 27% (10 of 37)
morphism.pyx: 20% (5 of 24)
pushout.py: 24% (19 of 77)
</pre><p>
The original purpose of this ticket was to provide full doctest coverage for <code>functor.pyx</code> and <code>pushout.py</code>. <strong>The doctest coverage of both files is now 100%</strong>.
</p>
<p>
However, the attempt to create meaningful doctests uncovered many bugs in various parts of Sage, and also motivated the implementation of coercion for various algebraic structures for which this has not been done before.
</p>
<p>
This a-posteriori ticket description lists the bugs killed and the features added by the patch, which should apply (with a little fuzz) after the patch from <a class="closed ticket" href="https://trac.sagemath.org/ticket/8807" title="enhancement: Adding support for morphisms to the category framework (closed: fixed)">#8807</a>. For more details on the bugs, see the comments below.
</p>
<ol><li>Bug: Creating <code>ForgetfulFunctor</code> fails.
</li></ol><blockquote>
<p>
Was:
</p>
<pre class="wiki">sage: abgrps = CommutativeAdditiveGroups()
sage: ForgetfulFunctor(abgrps, abgrps)
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
/home/king/SAGE/patches/doku/english/<ipython console> in <module>()
/home/king/SAGE/sage-4.3.1/local/lib/python2.6/site-packages/sage/categories/functor.so in sage.categories.functor.ForgetfulFunctor (sage/categories/functor.c:2083)()
TypeError: IdentityFunctor() takes exactly one argument (2 given)
</pre></blockquote>
<blockquote>
<p>
Now:
</p>
<pre class="wiki">sage: abgrps = CommutativeAdditiveGroups()
sage: ForgetfulFunctor(abgrps, abgrps)
The identity functor on Category of commutative additive groups
</pre></blockquote>
<ol start="2"><li>Bug: Applying <code>ForgetfulFunctor</code> returns <code>None</code>.
</li></ol><blockquote>
<p>
Was:
</p>
<pre class="wiki">sage: fields = Fields()
sage: rings = Rings()
sage: F = ForgetfulFunctor(fields,rings)
sage: F(QQ)
</pre></blockquote>
<blockquote>
<p>
Now:
</p>
<pre class="wiki">sage: fields = Fields()
sage: rings = Rings()
sage: F = ForgetfulFunctor(fields,rings)
sage: F(QQ)
Rational Field
</pre></blockquote>
<ol start="3"><li>Bug: Applying a functor does not complain if the argument is not contained in the domain.
</li></ol><blockquote>
<p>
Was:
</p>
<pre class="wiki">sage: fields = Fields()
sage: rings = Rings()
sage: F = ForgetfulFunctor(fields,rings)
# Yields None, see previous bug
sage: F(ZZ['x','y'])
</pre></blockquote>
<blockquote>
<p>
Now:
</p>
<pre class="wiki">sage: fields = Fields()
sage: rings = Rings()
sage: F = ForgetfulFunctor(fields,rings)
sage: F(ZZ['x','y'])
Traceback (most recent call last):
...
TypeError: x (=Multivariate Polynomial Ring in x, y over Integer Ring) is not in Category of fields
</pre></blockquote>
<ol start="4"><li>Bug: Comparing identity functor with any functor only checks domain and codomain
</li></ol><blockquote>
<p>
Was:
</p>
<pre class="wiki">sage: F = QQ['x'].construction()[0]
sage: F
Poly[x]
sage: F == IdentityFunctor(Rings())
False
sage: IdentityFunctor(Rings()) == F
True
</pre></blockquote>
<blockquote>
<p>
Now:
</p>
<pre class="wiki">sage: F = QQ['x'].construction()[0]
sage: F
Poly[x]
sage: F == IdentityFunctor(Rings())
False
sage: IdentityFunctor(Rings()) == F
False
</pre></blockquote>
<ol start="5"><li>Bug: Comparing identity functor with anything that is not a functor produces an error
</li></ol><blockquote>
<p>
Was:
</p>
<pre class="wiki">sage: IdentityFunctor(Rings()) == QQ
Traceback (most recent call last):
...
AttributeError: 'RationalField_with_category' object has no attribute 'domain'
</pre></blockquote>
<blockquote>
<p>
Now:
</p>
<pre class="wiki">sage: IdentityFunctor(Rings()) == QQ
False
</pre></blockquote>
<ol start="6"><li>Bug: The matrix functor is ill defined; moreover, ill-definedness does not result in an error.
</li></ol><blockquote>
<p>
Was:
</p>
<pre class="wiki">sage: F = MatrixSpace(ZZ,2,3).construction()[0]
sage: F(RR) in F.codomain()
False
# The codomain is wrong for non-square matrices!
sage: F.codomain()
Category of rings
</pre></blockquote>
<blockquote>
<p>
Now:
</p>
<pre class="wiki">sage: F = MatrixSpace(ZZ,2,3).construction()[0]
sage: F.codomain()
Category of commutative additive groups
sage: F(RR) in F.codomain()
True
sage: F = MatrixSpace(ZZ,2,2).construction()[0]
sage: F.codomain()
Category of rings
sage: F(RR) in F.codomain()
True
</pre></blockquote>
<ol start="7"><li>Bug: Wrong domain for <code>VectorFunctor</code>; and again, functors don't test if the domain is appropriate
</li></ol><blockquote>
<p>
Was:
</p>
<pre class="wiki">sage: F = FreeModule(ZZ,3).construction()[0]
sage: F
VectorFunctor
sage: F.domain()
Category of objects
sage: F.codomain()
Category of objects
sage: Set([1,2,3]) in F.domain()
True
sage: F(Set([1,2,3]))
Traceback (most recent call last):
...
AttributeError: 'Set_object_enumerated' object has no attribute 'is_commutative'
</pre></blockquote>
<blockquote>
<p>
Now:
</p>
<pre class="wiki">sage: F = FreeModule(ZZ,3).construction()[0]
sage: F
VectorFunctor
sage: F.domain()
Category of commutative rings
sage: Set([1,2,3]) in F.domain()
False
sage: F(Set([1,2,3]))
Traceback (most recent call last):
...
TypeError: x (={1, 2, 3}) is not in Category of commutative rings
</pre></blockquote>
<ol start="8"><li>Bug: <code>BlackBoxConstructionFunctor</code> is completely unusable
</li></ol><p>
<code>BlackBoxConstructionFunctor</code> should be a class, but is defined as a function. Moreover, the given init method is not using the init method of <code>ConstructionFunctor</code>. And the cmp method would raise an error if the second argument has no attribute <code>.box</code>.
</p>
<blockquote>
<p>
The following did not work at all:
</p>
<pre class="wiki">sage: from sage.categories.pushout import BlackBoxConstructionFunctor
sage: FG = BlackBoxConstructionFunctor(gap)
sage: FS = BlackBoxConstructionFunctor(singular)
sage: FG
BlackBoxConstructionFunctor
sage: FG(ZZ)
Integers
sage: FG(ZZ).parent()
Gap
sage: FS(QQ['t'])
// characteristic : 0
// number of vars : 1
// block 1 : ordering lp
// : names t
// block 2 : ordering C
sage: FG == FS
False
sage: FG == loads(dumps(FG))
True
</pre></blockquote>
<ol start="9"><li>Nitpicking: The <code>LocalizationFunctor</code> is nowhere used (yet)
</li></ol><p>
Hence, I removed it.
</p>
<ol start="10"><li>Bug / New Feature: Make completion and and fraction field construction functors commute.
</li></ol><p>
The result of them not commuting is the following coercion bug.
</p>
<blockquote>
<p>
Was:
</p>
<pre class="wiki">sage: R1.<x> = Zp(5)[]
sage: R2 = Qp(5)
sage: R2(1)+x
Traceback (most recent call last):
...
TypeError: unsupported operand parent(s) for '+': '5-adic Field with capped relative precision 20' and 'Univariate Polynomial Ring in x over 5-adic Ring with capped relative precision 20'
</pre></blockquote>
<blockquote>
<p>
Now:
</p>
<pre class="wiki">sage: R1.<x> = Zp(5)[]
sage: R2 = Qp(5)
sage: R2(1)+x
(1 + O(5^20))*x + (1 + O(5^20))
</pre></blockquote>
<ol start="11"><li>New feature: Make the completion functor work on some objects that do not provide a completion method.
</li></ol><p>
The idea is to use that the completion functor may commute with the construction of the given argument. That may safe the day.
</p>
<blockquote>
<p>
Was:
</p>
<pre class="wiki">sage: P.<x> = ZZ[]
sage: C = P.completion(x).construction()[0]
sage: R = FractionField(P)
sage: hasattr(R,'completion')
False
sage: C(R)
Traceback (most recent call last):
...
AttributeError: 'FractionField_generic' object has no attribute 'completion'
</pre></blockquote>
<blockquote>
<p>
Now:
</p>
<pre class="wiki">sage: P.<x> = ZZ[]
sage: C = P.completion(x).construction()[0]
sage: R = FractionField(P)
sage: hasattr(R,'completion')
False
sage: C(R)
Fraction Field of Power Series Ring in x over Integer Ring
</pre></blockquote>
<ol start="12"><li>Bug / new feature: Coercion for free modules, taking into account a user-defined inner product
</li></ol><blockquote>
<p>
Was:
</p>
<pre class="wiki">sage: P.<t> = ZZ[]
sage: M1 = FreeModule(P,3)
sage: M2 = QQ^3
sage: M2([1,1/2,1/3]) + M1([t,t^2+t,3]) # This is ok
(t + 1, t^2 + t + 1/2, 10/3)
sage: M3 = FreeModule(P,3, inner_product_matrix = Matrix(3,3,range(9)))
sage: M2([1,1/2,1/3]) + M3([t,t^2+t,3]) # This is ok
(t + 1, t^2 + t + 1/2, 10/3)
# The user defined inner product matrix is lost! Bug
sage: parent(M2([1,1/2,1/3]) + M3([t,t^2+t,3])).inner_product_matrix()
[1 0 0]
[0 1 0]
[0 0 1]
</pre></blockquote>
<blockquote>
<p>
Now:
</p>
<pre class="wiki">sage: parent(M2([1,1/2,1/3]) + M3([t,t^2+t,3])).inner_product_matrix()
[0 1 2]
[3 4 5]
[6 7 8]
</pre></blockquote>
<blockquote>
<p>
However, the real problem is that modules are not part of the coercion model. I tried to implement it, but that turned out to be a can of worms. So, <strong>I suggest to deal with it on a different ticket</strong>. Here is one bug that isn't removed, yet:
</p>
<pre class="wiki">sage: M4 = FreeModule(P,3, inner_product_matrix = Matrix(3,3,[1,1,1,0,1,1,0,0,1]))
sage: M3([t,1,t^2]) + M4([t,t^2+t,3]) # This should result in an error
(2*t, t^2 + t + 1, t^2 + 3)
</pre><p>
Note that there should be no coercion between <code>M3</code> and <code>M4</code>, since they have different user-defined inner product matrices.
</p>
</blockquote>
<ol start="13"><li>Bug / new feature: Quotient rings of univariate polynomial rings do not have a construction method.
</li></ol><blockquote>
<p>
Was:
</p>
<pre class="wiki">sage: P.<x> = QQ[]
sage: Q1 = P.quo([(x^2+1)^2*(x^2-3)])
sage: Q2 = P.quo([(x^2+1)^2*(x^5+3)])
sage: from sage.categories.pushout import pushout
sage: pushout(Q1,Q2)
Traceback (most recent call last):
...
CoercionException: No common base
</pre></blockquote>
<blockquote>
<p>
Now:
</p>
<pre class="wiki">sage: P.<x> = QQ[]
sage: Q1 = P.quo([(x^2+1)^2*(x^2-3)])
sage: Q2 = P.quo([(x^2+1)^2*(x^5+3)])
sage: from sage.categories.pushout import pushout
sage: pushout(Q1,Q2)
Univariate Quotient Polynomial Ring in xbar over Rational Field with modulus x^4 + 2*x^2 + 1
</pre></blockquote>
<ol start="14"><li>Insufficient coercion of quotient rings, if one modulus divides the other
</li></ol><blockquote>
<p>
Was:
</p>
<pre class="wiki">sage: P5.<x> = GF(5)[]
sage: Q = P5.quo([(x^2+1)^2])
sage: P.<x> = ZZ[]
sage: Q1 = P.quo([(x^2+1)^2*(x^2-3)])
sage: Q2 = P.quo([(x^2+1)^2*(x^5+3)])
sage: Q.has_coerce_map_from(Q1)
False
</pre></blockquote>
<blockquote>
<p>
Now: There is a coercion from <code>Q1</code> to <code>Q</code>.
</p>
</blockquote>
<ol start="15"><li>Coercion of <code>GF(p)</code> versus <code>Integers(p)</code>
</li></ol><p>
I am not sure if this is really a bug.
</p>
<blockquote>
<p>
Was:
</p>
<pre class="wiki">sage: from sage.categories.pushout import pushout
sage: pushout(GF(5), Integers(5))
Ring of integers modulo 5
</pre></blockquote>
<blockquote>
<p>
Now
</p>
<pre class="wiki">sage: from sage.categories.pushout import pushout
sage: pushout(GF(5), Integers(5))
Finite Field of size 5
</pre></blockquote>
<ol start="16"><li>Bug / new feature: Construction for QQbar was missing.
</li></ol><blockquote>
<p>
Now:
</p>
<pre class="wiki">sage: QQbar.construction()
(AlgebraicClosureFunctor, Rational Field)
</pre></blockquote>
<ol start="17"><li>Bug / new feature: Construction for number fields is missing.
</li></ol><p>
This became a rather complicated topic, including "coercions for embedded versus non-embedded number fields and coercion for an order from a coercion from the ambient field", "pushout for number fields", "comparison of fractional ideals", "identity of residue fields". See three discussions on sage-algebra and sage-nt
</p>
<ul><li><a class="ext-link" href="http://groups.google.com/group/sage-nt/browse_thread/thread/32b65a5173f43267"><span class="icon"></span>Bidirectional coercions</a>
</li><li><a class="ext-link" href="http://groups.google.com/group/sage-nt/browse_thread/thread/5c376dbf7e99ea97"><span class="icon"></span>Coercions for number fields</a>
</li><li><a class="ext-link" href="http://groups.google.com/group/sage-nt/browse_thread/thread/54c1e33872d14334"><span class="icon"></span>Comparison of fractional ideals</a>
</li></ul><p>
<span class="underline">Coercion</span>
</p>
<p>
Important for the discussion is: What will we do with embeddings?
</p>
<p>
Currently, the embedding of two number fields is used to construct a coercion (compatible with the embedding). Of course, the given embedding is also used as a coerce map.
</p>
<p>
It was discussed to additionally have a "forgetful" coercion from an embedded to a non-embedded number field.
</p>
<p>
It turned out that with bidirectional and forgetful coercions together, one can construct examples in which the coercions do not form a commutative diagram. Hence, we do <em>not</em> introduce forgetful coercions here.
</p>
<p>
However, some improvement of the existing implementation was needed.
</p>
<blockquote>
<p>
Was:
</p>
<pre class="wiki">sage: K.<r4> = NumberField(x^4-2)
sage: L1.<r2_1> = NumberField(x^2-2, embedding = r4**2)
sage: L2.<r2_2> = NumberField(x^2-2, embedding = -r4**2)
sage: r2_1+r2_2 # indirect doctest
ERROR: An unexpected error occurred while tokenizing input
The following traceback may be corrupted or invalid
The error message is: ('EOF in multi-line statement', (1109, 0))
ERROR: An unexpected error occurred while tokenizing input
The following traceback may be corrupted or invalid
The error message is: ('EOF in multi-line statement', (1109, 0))
...
sage: K.has_coerce_map_from(L1.maximal_order())
False # that's the wrong direction.
sage: L1.has_coerce_map_from(K.maximal_order())
True
</pre></blockquote>
<blockquote>
<p>
Now:
</p>
<pre class="wiki">sage: K.<r4> = NumberField(x^4-2)
sage: L1.<r2_1> = NumberField(x^2-2, embedding = r4**2)
sage: L2.<r2_2> = NumberField(x^2-2, embedding = -r4**2)
sage: r2_1+r2_2 # indirect doctest
0
sage: (r2_1+r2_2).parent() is L1
True
sage: (r2_2+r2_1).parent() is L2
True
sage: K.has_coerce_map_from(L1.maximal_order())
True
sage: L1.has_coerce_map_from(K.maximal_order())
False
</pre></blockquote>
<p>
<span class="underline">Pushout</span>
</p>
<blockquote>
<p>
Was:
</p>
<pre class="wiki">sage: P.<x> = QQ[]
sage: L.<b> = NumberField(x^8-x^4+1, embedding=CDF.0)
sage: M1.<c1> = NumberField(x^2+x+1, embedding=b^4-1)
sage: M2.<c2> = NumberField(x^2+1, embedding=-b^6)
sage: M1.coerce_map_from(M2)
sage: M2.coerce_map_from(M1)
sage: c1+c2; parent(c1+c2) #indirect doctest
Traceback (most recent call last):
...
TypeError: unsupported operand parent(s) for '+': 'Number Field in c1 with defining polynomial x^2 + x + 1' and 'Number Field in c2 with defining polynomial x^2 + 1'
sage: from sage.categories.pushout import pushout
sage: pushout(M1['x'],M2['x'])
Traceback (most recent call last):
...
CoercionException: No common base
</pre></blockquote>
<blockquote>
<p>
Now: Note that we will only have a pushout if the codomains of the embeddings are number fields. Hence, in the second example, we won't use <code>CDF</code> as a pushout.
</p>
<pre class="wiki">sage: P.<x> = QQ[]
sage: L.<b> = NumberField(x^8-x^4+1, embedding=CDF.0)
sage: M1.<c1> = NumberField(x^2+x+1, embedding=b^4-1)
sage: M2.<c2> = NumberField(x^2+1, embedding=-b^6)
sage: M1.coerce_map_from(M2)
sage: M2.coerce_map_from(M1)
sage: c1+c2; parent(c1+c2) #indirect doctest
-b^6 + b^4 - 1
Number Field in b with defining polynomial x^8 - x^4 + 1
sage: from sage.categories.pushout import pushout
sage: pushout(M1['x'],M2['x'])
Univariate Polynomial Ring in x over Number Field in b with defining polynomial x^8 - x^4 + 1
sage: K.<a> = NumberField(x^3-2, embedding=CDF(1/2*I*2^(1/3)*sqrt(3) - 1/2*2^(1/3)))
sage: L.<b> = NumberField(x^6-2, embedding=1.1)
sage: L.coerce_map_from(K)
sage: K.coerce_map_from(L)
sage: pushout(K,L)
Traceback (most recent call last):
...
CoercionException: ('Ambiguous Base Extension', Number Field in a with defining polynomial x^3 - 2, Number Field in b with defining polynomial x^6 - 2)
</pre></blockquote>
<p>
<span class="underline">Comparison of fractional ideals / identity of Residue Fields</span>
</p>
<blockquote>
<p>
Fractional ideals have a <code>__cmp__</code> method that only took into account the Hermite normal form. Originally, the comparison of fractional ideals by "==" and by "cmp" yields different results. Since "==" of fractional ideals is used for caching residue fields, but "cmp" was used for comparing residue fields, the residue fields did not provide unique parents.
</p>
</blockquote>
<blockquote>
<p>
Was:
</p>
<pre class="wiki">sage: L.<b> = NumberField(x^8-x^4+1)
sage: F_2 = L.fractional_ideal(b^2-1)
sage: F_4 = L.fractional_ideal(b^4-1)
sage: F_2==F_4
True
sage: K.<r4> = NumberField(x^4-2)
sage: L.<r4> = NumberField(x^4-2, embedding=CDF.0)
sage: FK = K.fractional_ideal(K.0)
sage: FL = L.fractional_ideal(L.0)
sage: FK != FL
True
sage: RL = ResidueField(FL)
sage: RK = ResidueField(FK)
sage: RK is RL
False
sage: RK == RL
True
</pre></blockquote>
<blockquote>
<p>
Now:
</p>
<pre class="wiki">sage: L.<b> = NumberField(x^8-x^4+1)
sage: F_2 = L.fractional_ideal(b^2-1)
sage: F_4 = L.fractional_ideal(b^4-1)
sage: F_2==F_4
True
sage: K.<r4> = NumberField(x^4-2)
sage: L.<r4> = NumberField(x^4-2, embedding=CDF.0)
sage: FK = K.fractional_ideal(K.0)
sage: FL = L.fractional_ideal(L.0)
sage: FK != FL
True
sage: RL = ResidueField(FL)
sage: RK = ResidueField(FK)
sage: RK is RL
False
sage: RK == RL
False
</pre></blockquote>
<blockquote>
<p>
Since <code>RL</code> is defined with the embedded field <code>L</code>, not with the unembedded <code>K</code>, there is no coercion from the order of <code>K</code> to <code>RL</code>. However, <em>conversion</em> works (this used to fail!):
</p>
</blockquote>
<pre class="wiki">sage: OK = K.maximal_order()
sage: RL.has_coerce_map_from(OK)
False
sage: RL(OK.1)
0
</pre><blockquote>
<p>
Note that I also had to change some arithmetic stuff in the <code>_tate</code> method of elliptic curves: The old implementation relied on the assumption that fractional ideals in an embedded field and in a non-embedded field can't be equal. This assumption should still hold (since we do not introduce forgetful coercion), but I think it is OK to keep the change in _tate.
</p>
</blockquote>
<p>
<strong>Apply:</strong>
</p>
<ol><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/8800/8800_functor_pushout_doc_and_fixes.patch" title="Attachment '8800_functor_pushout_doc_and_fixes.patch' in Ticket #8800">8800_functor_pushout_doc_and_fixes.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/8800/8800_functor_pushout_doc_and_fixes.patch" title="Download"></a>
</li><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/8800/referee.patch" title="Attachment 'referee.patch' in Ticket #8800">referee.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/8800/referee.patch" title="Download"></a>
</li></ol><p>
<strong>Dependencies:</strong> <a class="closed ticket" href="https://trac.sagemath.org/ticket/10677" title="enhancement: Improve PARI interface for relative number fields (closed: fixed)">#10677</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/2329" title="enhancement: Add interface to PARI's rnfisnorm() (closed: fixed)">#2329</a>.
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/8800
Trac 1.1.6SimonKingWed, 28 Apr 2010 08:51:17 GMTdescription changed
https://trac.sagemath.org/ticket/8800#comment:1
https://trac.sagemath.org/ticket/8800#comment:1
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/8800?action=diff&version=1">diff</a>)
</li>
</ul>
TicketSimonKingWed, 28 Apr 2010 09:01:41 GMT
https://trac.sagemath.org/ticket/8800#comment:2
https://trac.sagemath.org/ticket/8800#comment:2
<p>
Shouldn't the following raise an error, since the argument is not contained in the domain? Instead, it returns <code></code>None<code></code>.
</p>
<pre class="wiki">sage: fields = Fields()
sage: rings = Rings()
sage: F = ForgetfulFunctor(fields,rings)
sage: F(ZZ['x','y'])
</pre>
TicketSimonKingWed, 28 Apr 2010 09:06:39 GMT
https://trac.sagemath.org/ticket/8800#comment:3
https://trac.sagemath.org/ticket/8800#comment:3
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:2" title="Comment 2">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Shouldn't the following raise an error, since the argument is not contained in the domain? Instead, it returns <code></code>None<code></code>.
</p>
</blockquote>
<p>
And this is because the generic <code></code><span class="underline">call</span><code></code> method of <code></code>Functor<code></code> has <em>no</em> return value! That's clearly a bug.
</p>
TicketSimonKingWed, 28 Apr 2010 09:51:13 GMT
https://trac.sagemath.org/ticket/8800#comment:4
https://trac.sagemath.org/ticket/8800#comment:4
<p>
Next bug:
</p>
<pre class="wiki">sage: F = QQ['x'].construction()[0]
sage: F
Poly[x]
sage: F == IdentityFunctor(Rings())
False
sage: IdentityFunctor(Rings()) == F
True
</pre><p>
This is since the cmp method of <code></code>IdentityFunctor_generic<code></code> only checks whether domain and codomain coincide, but doesn't check the type of the functor.
</p>
<p>
Even worse, comparison it may raise an error - how unpythonic!
</p>
<pre class="wiki">sage: IdentityFunctor(Rings()) == QQ
---------------------------------------------------------------------------
AttributeError Traceback (most recent call last)
/home/king/SAGE/patches/doku/english/<ipython console> in <module>()
/home/king/SAGE/sage-4.3.1/local/lib/python2.6/site-packages/sage/categories/functor.so in sage.categories.functor.ForgetfulFunctor_generic.__cmp__ (sage/categories/functor.c:1429)()
/home/king/SAGE/sage-4.3.1/local/lib/python2.6/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.__getattr__ (sage/structure/parent.c:5064)()
/home/king/SAGE/sage-4.3.1/local/lib/python2.6/site-packages/sage/structure/parent.so in sage.structure.parent.getattr_from_other_class (sage/structure/parent.c:2738)()
/home/king/SAGE/sage-4.3.1/local/lib/python2.6/site-packages/sage/structure/parent.so in sage.structure.parent.raise_attribute_error (sage/structure/parent.c:2610)()
AttributeError: 'RationalField_with_category' object has no attribute 'domain'
</pre>
TicketSimonKingWed, 28 Apr 2010 11:16:40 GMT
https://trac.sagemath.org/ticket/8800#comment:5
https://trac.sagemath.org/ticket/8800#comment:5
<p>
I think the call method of the class Functor is not cleanly implemented.
</p>
<p>
It seems intended that the user does not implement the call method. Instead s/he should implement _apply_functor, which is supposed to return an object in the functor's codomain.
</p>
<p>
Before using _apply_functor, the default call method tests whether the argument belongs to the domain. If this is not the case, it <strong>coerces</strong> the argument into the domain. I don't think that this is always wanted. E.g., the forgetful functor from fields to rings, when applied to the integer ring, currently returns the rational field (so, the forgetful functor <em>adds</em> structure), since the default call method first coerces the integer ring into the category of fields (which is done by the fraction field construction functor).
</p>
<p>
I suggest to introduce a method _coerce_into_domain. By default, it returns its argument without change. If the user wants coercion into the domain (e.g. Integer Ring --> Rational Field), then s/he must implement it here.
</p>
<p>
The default call method should first apply _coerce_into_domain, check whether the result is in the domain (raise an error if this is not the case), then use _apply_functor, and check whether the result is in the codomain (and raise an error otherwise). And of course it should return the result (which was forgotten!).
</p>
<p>
Thoughts?
</p>
TicketSimonKingWed, 28 Apr 2010 18:24:46 GMT
https://trac.sagemath.org/ticket/8800#comment:6
https://trac.sagemath.org/ticket/8800#comment:6
<p>
It meanwhile seems to me that an overhaul of the category framework is needed, in order to properly support working with morphisms and functors. I therefore opened <a class="closed ticket" href="https://trac.sagemath.org/ticket/8807" title="enhancement: Adding support for morphisms to the category framework (closed: fixed)">#8807</a>.
</p>
<p>
It could be that this ticket will eventually be 'absorbed' by <a class="closed ticket" href="https://trac.sagemath.org/ticket/8807" title="enhancement: Adding support for morphisms to the category framework (closed: fixed)">#8807</a>. We will see...
</p>
TicketSimonKingSun, 02 May 2010 19:11:29 GMT
https://trac.sagemath.org/ticket/8800#comment:7
https://trac.sagemath.org/ticket/8800#comment:7
<p>
I think it would be better to base this ticket on <a class="closed ticket" href="https://trac.sagemath.org/ticket/8807" title="enhancement: Adding support for morphisms to the category framework (closed: fixed)">#8807</a>, since I believe that <a class="closed ticket" href="https://trac.sagemath.org/ticket/8807" title="enhancement: Adding support for morphisms to the category framework (closed: fixed)">#8807</a> should be merged soon anyway.
</p>
<p>
Continuing with the doc tests, I think I found another bug, namely in the <code></code>merge<code></code> method of the Quotient construction functor:
</p>
<pre class="wiki">sage: Q15,R = (ZZ.quo(15*ZZ)).construction()
sage: Q15
QuotientFunctor
sage: Q35,R = (ZZ.quo(35*ZZ)).construction()
sage: Q35
QuotientFunctor
sage: Q15.merge(Q35) is None
True
sage: from sage.categories.pushout import pushout
sage: pushout(ZZ.quo(15*ZZ),ZZ.quo(35*ZZ))
---------------------------------------------------------------------------
CoercionException Traceback (most recent call last)
/home/SimonKing/<ipython console> in <module>()
/usr/local/sage/local/lib/python2.6/site-packages/sage/categories/pushout.pyc in pushout(R, S)
1099 else:
1100 # Otherwise, we cannot proceed.
-> 1101 raise CoercionException, ("Ambiguous Base Extension", R, S)
1102
1103 return all(Z)
CoercionException: ('Ambiguous Base Extension', Ring of integers modulo 15, Ring of integers modulo 35)
</pre><p>
The reason is that internally <code></code>Q35.I + Q15.I<code></code> is tried, but this raises an error. It works with <code></code>Q35.I.gcd(Q15.I)<code></code>, though. If I do this (in a patch that I will hopefully post in a few days, one gets (as one <em>should</em>, if I am not mistaken)
</p>
<pre class="wiki">sage: pushout(ZZ.quo(15*ZZ),ZZ.quo(35*ZZ))
Ring of integers modulo 5
</pre>
TicketSimonKingFri, 07 May 2010 14:30:55 GMT
https://trac.sagemath.org/ticket/8800#comment:8
https://trac.sagemath.org/ticket/8800#comment:8
<p>
Another bug that I plan to remove:
</p>
<pre class="wiki">sage: F = MatrixSpace(ZZ,2,3).construction()[0]
sage: F(RR) in F.codomain()
False
</pre><p>
The problem is that the codomain of <code></code>F<code></code> is supposed to be the category of rings, even for non-square matrices. I'll change it to the following:
</p>
<pre class="wiki">sage: MatrixSpace(ZZ,2,3).construction()[0].codomain()
Category of commutative additive groups
sage: MatrixSpace(ZZ,2,2).construction()[0].codomain()
Category of rings
</pre><p>
I'd actually like to have the category of modules (rather than of additive groups), but this would require a ring over which the module is defined (and which the functor obviously can't know).
</p>
TicketSimonKingFri, 07 May 2010 14:49:48 GMT
https://trac.sagemath.org/ticket/8800#comment:9
https://trac.sagemath.org/ticket/8800#comment:9
<p>
The next one:
</p>
<pre class="wiki">sage: F = FreeModule(ZZ,3).construction()[0]
sage: F
VectorFunctor
sage: F.domain()
Category of objects
sage: F.codomain()
Category of objects
sage: Set([1,2,3]) in F.domain()
True
sage: F(Set([1,2,3]))
Traceback (most recent call last):
...
AttributeError: 'Set_object_enumerated' object has no attribute 'is_commutative'
</pre><p>
Since the functor calls the <code></code><a class="missing wiki">FreeModule?</a><code></code> constructor, and since this constructor expects a commutative ring, the Vector functor should go from the category of commutative rings to the category of commutative additive groups (since the category of modules requires naming a base ring).
</p>
TicketSimonKingFri, 07 May 2010 19:26:52 GMT
https://trac.sagemath.org/ticket/8800#comment:10
https://trac.sagemath.org/ticket/8800#comment:10
<p>
Next bug:
</p>
<p>
<code></code><a class="missing wiki">BlackBoxConstructionFunctor?</a><code></code> should be a class, but is defined as a function. Moreover, the given init method is not using the init method of <code></code><a class="missing wiki">ConstructionFunctor?</a><code></code>. And the cmp method would raise an error if the second argument has no attribute <code></code>.box<code></code>.
</p>
TicketSimonKingFri, 07 May 2010 20:02:31 GMT
https://trac.sagemath.org/ticket/8800#comment:11
https://trac.sagemath.org/ticket/8800#comment:11
<p>
Merging <code></code><a class="missing wiki">AlgebraicClosureFunctor?</a><code></code> with <em>anything</em> else always yields the <code></code><a class="missing wiki">AlgebraicClosureFunctor?</a><code></code>. I doubt that this was intended. There should be a merging with an <code></code><a class="missing wiki">AlgebraicExtensionFunctor?</a><code></code>, though.
</p>
TicketSimonKingFri, 07 May 2010 20:14:32 GMT
https://trac.sagemath.org/ticket/8800#comment:12
https://trac.sagemath.org/ticket/8800#comment:12
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:11" title="Comment 11">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
... There should be a merging with an <code></code><a class="missing wiki">AlgebraicExtensionFunctor?</a><code></code>, though.
</p>
</blockquote>
<p>
... which is nowhere used, though. I think the method <code></code>construction<code></code> for number fields should be defined.
</p>
TicketSimonKingSat, 08 May 2010 09:39:41 GMT
https://trac.sagemath.org/ticket/8800#comment:13
https://trac.sagemath.org/ticket/8800#comment:13
<pre class="wiki">sage: P.<x> = QQ[]
sage: CC.extension(x^3+x^2+1,'a')
Univariate Quotient Polynomial Ring in a over Complex Field with 53 bits of precision with modulus a^3 + a^2 + 1.00000000000000
sage: CDF.extension(x^3+x^2+1,'a')
Univariate Quotient Polynomial Ring in a over Complex Double Field with modulus a^3 + a^2 + 1.0
sage: QQbar.extension(x^3+x^2+1,'a')
Univariate Quotient Polynomial Ring in a over Algebraic Field with modulus a^3 + a^2 + 1
</pre><p>
Aren't the three above fields algebraically complete? So, I guess the <code></code>extension<code></code> method should be modified to take this into account.
</p>
TicketSimonKingFri, 14 May 2010 13:59:06 GMT
https://trac.sagemath.org/ticket/8800#comment:14
https://trac.sagemath.org/ticket/8800#comment:14
<p>
Concerning algebraic extension of algebraically complete fields: sage-devel expressed the opinion that it is better to do the construction (namely quotient of a univariate polynomial ring) in any case. So, I leave it as it is.
</p>
<p>
Here is another problem:
</p>
<pre class="wiki">sage: R1.<x> = Zp(5)[]
sage: R2 = Qp(5)
sage: R2(1)+x
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
/home/SimonKing/<ipython console> in <module>()
/usr/local/sage/local/lib/python2.6/site-packages/sage/structure/element.so in sage.structure.element.RingElement.__add__ (sage/structure/element.c:10830)()
/usr/local/sage/local/lib/python2.6/site-packages/sage/structure/coerce.so in sage.structure.coerce.CoercionModel_cache_maps.bin_op (sage/structure/coerce.c:6966)()
TypeError: unsupported operand parent(s) for '+': '5-adic Field with capped relative precision 20' and 'Univariate Polynomial Ring in x over 5-adic Ring with capped relative precision 20'
</pre><p>
The reason is
</p>
<pre class="wiki">sage: from sage.categories.pushout import pushout
sage: pushout(R1,R2)
---------------------------------------------------------------------------
CoercionException Traceback (most recent call last)
/home/SimonKing/<ipython console> in <module>()
/usr/local/sage/local/lib/python2.6/site-packages/sage/categories/pushout.pyc in pushout(R, S)
1109 # make sense, and in this case simply want to return that a pushout
1110 # couldn't be found.
-> 1111 raise CoercionException(ex)
1112
1113
CoercionException: 'pAdicFieldCappedRelative' object has no attribute 'completion'
</pre><p>
Rather than implementing a completion of p-adic fields, I suggest to give the construction functors of fraction fields and of completions the same rank. This would already suffice (together with the existing merge method of the completion functor) so that one has
</p>
<pre class="wiki">sage: R1.<x> = Zp(5)[]
sage: R2 = Qp(5)
sage: R2(1) + x
(1 + O(5^20))*x + (1 + O(5^20))
</pre><p>
Note that there is an additional problem, namely that there is no coercion from a p-adic field of high precision to a p-adic field of lower precision. I hope sage-devel will answer whether this issue is worth a separate ticket.
</p>
TicketSimonKingFri, 14 May 2010 14:59:02 GMT
https://trac.sagemath.org/ticket/8800#comment:15
https://trac.sagemath.org/ticket/8800#comment:15
<p>
PS:
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:14" title="Comment 14">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Rather than implementing a completion of p-adic fields, I suggest to give the construction functors of fraction fields and of completions the same rank...
</p>
</blockquote>
<p>
... since they commute anyway. I guess it wouldn't harm to implement the <code></code>commutes<code></code> method as well.
</p>
<p>
I now consider the Localization functor. It uses the method "localize", but:
</p>
<pre class="wiki">sage: search_def('localize')
sage: search_src('localize')
categories/pushout.py:1294: return R.localize(t)
libs/singular/option.pyx:367: This object localizes changes to options.
</pre><p>
In other words, there is no class that has a localize method. So, I guess it is safe to comment the Localization functor out.
</p>
TicketjasonFri, 14 May 2010 15:55:14 GMT
https://trac.sagemath.org/ticket/8800#comment:16
https://trac.sagemath.org/ticket/8800#comment:16
<p>
Wow, I count 7 bugs in the comments above! What a testament for the need for writing good doctests (and to how careful you are!)
</p>
TicketSimonKingFri, 14 May 2010 18:24:11 GMT
https://trac.sagemath.org/ticket/8800#comment:17
https://trac.sagemath.org/ticket/8800#comment:17
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:14" title="Comment 14">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Note that there is an additional problem, namely that there is no coercion from a p-adic field of high precision to a p-adic field of lower precision. I hope sage-devel will answer whether this issue is worth a separate ticket.
</p>
</blockquote>
<p>
Sage-devel (more precisely Robert Bradshaw) wrote that the meaning of "precision" is different for completion at Infinity and at finite primes, and it makes sense that sometimes the precision is non-decreasing and sometimes non-increasing under coercion.
</p>
<p>
So, I guess I have to modify the merge method of the Completion funtor, rather than the _coerce_map_from method of p-adic rings.
</p>
TicketSimonKingSat, 15 May 2010 12:22:58 GMT
https://trac.sagemath.org/ticket/8800#comment:18
https://trac.sagemath.org/ticket/8800#comment:18
<p>
I noticed the following:
</p>
<pre class="wiki">sage: P.<x> = ZZ[]
sage: C = P.completion(x).construction()[0]
sage: R = FractionField(P)
sage: hasattr(R,'completion')
False
sage: C(R)
Traceback (most recent call last):
...
AttributeError: 'FractionField_generic' object has no attribute 'completion'
</pre><p>
This is since the completion functor simply tries to call the completion method of its argument. However, one can use that the fraction field construction functor and the completion functor commute.
</p>
<p>
So, I first try to apply a completion method of the argument, R. If it fails with an <a class="missing wiki">AttributeError?</a> or <a class="missing wiki">NotImplementedError?</a>, I look at R's construction (F,R1). If F merges with completion, then I apply the result of merging to R1. Otherwise, if the completion commutes with F, I try to first apply the completion to R1 and then apply F to the result, and obtain:
</p>
<pre class="wiki">sage: C(R)
Fraction Field of Power Series Ring in x over Integer Ring
</pre><p>
Note that this would <em>not</em> be the first place where merging and commutation of construction functors is used outside the <code></code>pushout<code></code> function. The other place is the construction of infinite polynomial rings, which I wrote as well. Indeed I believe that construction functors should be used more intensely...
</p>
TicketSimonKingMon, 17 May 2010 09:03:12 GMT
https://trac.sagemath.org/ticket/8800#comment:19
https://trac.sagemath.org/ticket/8800#comment:19
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:18" title="Comment 18">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
...
</p>
<pre class="wiki">sage: C(R)
Fraction Field of Power Series Ring in x over Integer Ring
</pre></blockquote>
<p>
I believe that the fraction field of a power series ring over a base ring <code></code>B<code></code> should be identical with the Laurent series ring over the fraction field of <code></code>B<code></code>. This is implemented in ticket <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/8972" title="defect: Inversion and fraction fields for power series rings (needs_work)">#8972</a>.
</p>
<p>
I am tempted to say "let's wait until <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/8972" title="defect: Inversion and fraction fields for power series rings (needs_work)">#8972</a> is refereed", because the doc tests I am constructing here will depend on whether <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/8972" title="defect: Inversion and fraction fields for power series rings (needs_work)">#8972</a> gets merged or not.
</p>
<p>
What is the policy in those cases? Should I simply continue the work on the doc tests and care about <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/8972" title="defect: Inversion and fraction fields for power series rings (needs_work)">#8972</a> later?
</p>
TicketSimonKingMon, 17 May 2010 12:53:51 GMT
https://trac.sagemath.org/ticket/8800#comment:20
https://trac.sagemath.org/ticket/8800#comment:20
<p>
Currently, the construction functors for free modules and for matrix spaces have the same rank, but they do not commute and do not merge. Hence, the following goes boom:
</p>
<pre class="wiki">sage: from sage.categories.pushout import pushout
sage: pushout(QQ^3,MatrixSpace(QQ,3))
---------------------------------------------------------------------------
CoercionException Traceback (most recent call last)
...
CoercionException: ('Ambiguous Base Extension', Vector space of dimension 3 over Rational Field, Full MatrixSpace of 3 by 3 dense matrices over Rational Field)
</pre><p>
I think this pushout should exist. But what should result?
</p>
<ol><li><code></code><a class="missing wiki">MatrixSpace?</a>(QQ,3)<sup>3</sup><code></code> resp. <code></code><a class="missing wiki">FreeModule?</a>(<a class="missing wiki">MatrixSpace?</a>(QQ,3),3)<code></code>. This is currently not possible, since <code></code>MatrixSpace_generic<code></code> has no attribute <code></code>is_commutative<code></code>.
</li></ol><ol start="2"><li><code></code><a class="missing wiki">MatrixSpace?</a>(QQ<sup>3</sup>,3)<code></code> makes no sense, as <code></code>QQ<sup>3</sup><code></code> is no ring.
</li></ol><ol start="3"><li><code></code><a class="missing wiki">MatrixSpace?</a>(QQ,27)<code></code> makes not much sense, as I don't see coercion maps.
</li></ol><p>
So, probably it is solution number 1, which at least requires to implement an <code></code>is_commutative<code></code> method, resp. to first test for the presence of such method in <code></code><a class="missing wiki">FreeModule?</a><code></code>. I think I'll go for it.
</p>
TicketSimonKingMon, 17 May 2010 12:58:55 GMT
https://trac.sagemath.org/ticket/8800#comment:21
https://trac.sagemath.org/ticket/8800#comment:21
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:20" title="Comment 20">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
So, probably it is solution number 1, which at least requires to implement an <code></code>is_commutative<code></code> method, resp. to first test for the presence of such method in <code></code><a class="missing wiki">FreeModule?</a><code></code>. I think I'll go for it.
</p>
</blockquote>
<p>
Oops, this is nonsense. The <code></code><a class="missing wiki">FreeModule?</a><code></code> constructor expects a commutative ring. So, solution 1. is no solution. I will change the constructor so that it is first tested whether the <code></code>is_commutative<code></code> method exists, so that the error message is clearer, but apart from that, it is OK that the pushout does not exist.
</p>
TicketSimonKingMon, 17 May 2010 13:22:44 GMT
https://trac.sagemath.org/ticket/8800#comment:22
https://trac.sagemath.org/ticket/8800#comment:22
<p>
I believe that free modules of the same rank but with different inner product matrix should not allow coercion. Hence, I think the following is a bug:
</p>
<pre class="wiki">sage: P.<t> = ZZ[]
sage: M1 = FreeModule(P,3)
sage: M2 = QQ^3
sage: M2([1,1/2,1/3]) + M1([t,t^2+t,3]) # This is ok
(t + 1, t^2 + t + 1/2, 10/3)
sage: M3 = FreeModule(P,3, inner_product_matrix = Matrix(3,3,range(9)))
sage: M2([1,1/2,1/3]) + M3([t,t^2+t,3]) # This should result in an error
(t + 1, t^2 + t + 1/2, 10/3)
</pre><p>
This inappropriate coercion can be avoided by modifying the merge method of the construction functors, so that the inner product matrices are used for comparison as well.
</p>
<p>
But I acknowledge that other people might think that a coercion should exist. Perhaps I shall ask on sage-algebra...
</p>
TicketSimonKingMon, 17 May 2010 17:43:50 GMT
https://trac.sagemath.org/ticket/8800#comment:23
https://trac.sagemath.org/ticket/8800#comment:23
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:22" title="Comment 22">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
...
But I acknowledge that other people might think that a coercion should exist. Perhaps I shall ask on sage-algebra...
</p>
</blockquote>
<p>
sage-algebra (John Cremona and William Stein) answered that the inner product is an important structure if and <em>only</em> if it is explicitly defined by the user. Hence, in the above example with <code></code>M2<code></code> and <code></code>M3<code></code>, no error should be raised, since <code></code>M2<code></code> has no user-defined inner product. But if <code></code>M2<code></code> was <em>explicitly</em> be provided with the standard inner product, then an error should be raised.
</p>
<p>
That's easy to implement: The <code></code>construction()<code></code> method of the modules returns a <code></code><a class="missing wiki">VectorFunctor?</a><code></code>, and this one carries the inner product matrix (if provided by the user) or None. And two <code></code><a class="missing wiki">VectorFunctor?</a><code></code>s carrying different inner product matrices will not be merged.
</p>
TicketSimonKingTue, 18 May 2010 15:14:58 GMT
https://trac.sagemath.org/ticket/8800#comment:24
https://trac.sagemath.org/ticket/8800#comment:24
<p>
Next issue: Quotient rings of univariate polynomial rings did not have a construction method. I am implementing it, so that one has:
</p>
<pre class="wiki">sage: P.<t>=ZZ[]
sage: Q = P.quo(5+t^2)
sage: F,R = Q.construction()
sage: F(R) == Q
True
sage: P.<t> = GF(3)[]
sage: Q = P.quo([2+t^2])
sage: F,R = Q.construction()
sage: F(R) == Q
True
</pre>
TicketSimonKingTue, 18 May 2010 16:38:06 GMT
https://trac.sagemath.org/ticket/8800#comment:25
https://trac.sagemath.org/ticket/8800#comment:25
<p>
I am now almost finished with the doc tests for pushout.py.
</p>
<p>
The soon-to-be-submitted patch is already quite big, and comprises various bug fixes. I suggest that this ticket will mainly be about pushout.py, and the other files will be done on a separate ticket.
</p>
<p>
Here are three more bugs. Number one:
</p>
<pre class="wiki">sage: sage: P.<x> = QQ[]
sage: P.<x> = QQ[]
sage: Q1 = P.quo([(x^2+1)^2*(x^2-3)])
sage: Q2 = P.quo([(x^2+1)^2*(x^5+3)])
sage: from sage.categories.pushout import pushout
sage: pushout(Q1,Q2)
---------------------------------------------------------------------------
CoercionException Traceback (most recent call last)
/home/king/SAGE/work/invarianten/<ipython console> in <module>()
/home/king/SAGE/sage-4.3.1/local/lib/python2.6/site-packages/sage/categories/pushout.pyc in pushout(R, S)
1037
1038 else:
-> 1039 raise CoercionException, "No common base"
1040
1041 # Rc is a list of functors from Z to R and Sc is a list of functors from Z to S
CoercionException: No common base
</pre><p>
This I can fix. The problem is that the quotient rings have no proper <code></code>construction()<code></code> method.
</p>
<p>
Number 2, continuing the above example:
</p>
<pre class="wiki">sage: Q = P.quo([(x^2+1)^2])
sage: Q.has_coerce_map_from(Q1)
False
sage: Q.has_coerce_map_from(Q2)
False
</pre><p>
This is wrong since the modulus of Q divides the modulus of Q1 and Q2. Actually Q is supposed to be the pushout of Q1 and Q2.
</p>
<p>
Number three:
</p>
<pre class="wiki">sage: Q(Q1.gen())
ERROR: An unexpected error occurred while tokenizing input
The following traceback may be corrupted or invalid
The error message is: ('EOF in multi-line statement', (932, 0))
...
TypeError: Unable to coerce xbar (<class 'sage.rings.polynomial.polynomial_quotient_ring_element.PolynomialQuotientRingElement'>) to Rational
</pre><p>
But I guess these last two errors should be on a different ticket.
</p>
TicketSimonKingTue, 18 May 2010 23:04:09 GMTstatus changed
https://trac.sagemath.org/ticket/8800#comment:26
https://trac.sagemath.org/ticket/8800#comment:26
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
</ul>
<ul><li>The patch is to be applied <em>after the patches from</em> <a class="closed ticket" href="https://trac.sagemath.org/ticket/8807" title="enhancement: Adding support for morphisms to the category framework (closed: fixed)">#8807</a>.
</li></ul><ul><li>It raises the <strong>doctest coverage of sage.categories.functor and sage.categories.pushout to 100%</strong> and occasionally adds doc tests in other places.
</li></ul><ul><li>It fixes numerous bugs related with coercion, as indicated in the posts above.
</li></ul><p>
Constructing doc tests for pushout.py and functor.pyx revealed many bugs, so that I needed to change
</p>
<pre class="wiki">sage/structure/parent.pyx
sage/rings/ring.pyx
sage/rings/rational_field.py
sage/rings/quotient_ring.py
sage/rings/qqbar.py
sage/rings/polynomial/polynomial_quotient_ring.py
sage/rings/number_field/number_field.py
sage/modules/free_module.py
sage/categories/pushout.py
sage/categories/functor.pyx
</pre><p>
The doc tests for all these files still pass.
</p>
<p>
I think it would make no sense to put more on this ticket. The work on doc tests in map.pyx, morphism.pyx and action.pyx will be moved to a different ticket.
</p>
TicketrobertwbWed, 19 May 2010 03:55:01 GMT
https://trac.sagemath.org/ticket/8800#comment:27
https://trac.sagemath.org/ticket/8800#comment:27
<p>
Wow, this is looking very good!
</p>
<p>
<a class="missing wiki">MatrixFunctor?</a>.<span class="underline">init</span>, is there not a module category that could be used in place of <code>CommutativeAdditiveGroups</code>? I guess if tbe basering is unknown then that's more difficult.
</p>
<p>
Missing periods on <code>VectorFunctor.__cmp__</code> and <code>VectorFunctor.merge</code>. I agree with the logic for that merge function.
</p>
<p>
That's all I've seen so far (and I've read most of the patch.) You've fixed a lot of bugs too. Pending doctests passing, I'd say this is ready for a positive review.
</p>
TicketSimonKingWed, 19 May 2010 08:23:57 GMT
https://trac.sagemath.org/ticket/8800#comment:28
https://trac.sagemath.org/ticket/8800#comment:28
<p>
Hi Robert!
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:27" title="Comment 27">robertwb</a>:
</p>
<blockquote class="citation">
<p>
<a class="missing wiki">MatrixFunctor?</a>.<span class="underline">init</span>, is there not a module category that could be used in place of <code>CommutativeAdditiveGroups</code>? I guess if tbe basering is unknown then that's more difficult.
</p>
</blockquote>
<p>
Yes, <code>Modules()</code> requires a base ring. There is currently no category of modules, but only a category of R-modules for any ring R. This is why I used <code>CommutativeAdditiveGroups()</code> in several cases.
</p>
<blockquote class="citation">
<p>
Missing periods on <code>VectorFunctor.__cmp__</code> and <code>VectorFunctor.merge</code>.
</p>
</blockquote>
<p>
Missing where? In the doc string?
</p>
<p>
Concerning positive review, note that technically this ticket depends on <a class="closed ticket" href="https://trac.sagemath.org/ticket/8807" title="enhancement: Adding support for morphisms to the category framework (closed: fixed)">#8807</a>, which has no review yet.
</p>
<p>
Best regards,
Simon
</p>
TicketrobertwbWed, 19 May 2010 10:03:55 GMT
https://trac.sagemath.org/ticket/8800#comment:29
https://trac.sagemath.org/ticket/8800#comment:29
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:28" title="Comment 28">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Hi Robert!
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:27" title="Comment 27">robertwb</a>:
</p>
<blockquote class="citation">
<p>
<a class="missing wiki">MatrixFunctor?</a>.<span class="underline">init</span>, is there not a module category that could be used in place of <code>CommutativeAdditiveGroups</code>? I guess if tbe basering is unknown then that's more difficult.
</p>
</blockquote>
<p>
Yes, <code>Modules()</code> requires a base ring. There is currently no category of modules, but only a category of R-modules for any ring R. This is why I used <code>CommutativeAdditiveGroups()</code> in several cases.
</p>
</blockquote>
<p>
Hmm... does it make sense to have a category of Modules (over any basering)?
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
Missing periods on <code>VectorFunctor.__cmp__</code> and <code>VectorFunctor.merge</code>.
</p>
</blockquote>
<p>
Missing where? In the doc string?
</p>
</blockquote>
<p>
Yes, there were a couple of sentences without ending periods. Nothing major.
</p>
<blockquote class="citation">
<p>
Concerning positive review, note that technically this ticket depends on <a class="closed ticket" href="https://trac.sagemath.org/ticket/8807" title="enhancement: Adding support for morphisms to the category framework (closed: fixed)">#8807</a>, which has no review yet.
</p>
</blockquote>
<p>
Yep. I started to look at that one too, and will review it if no one beats me too it when I have another spare moment (maybe the upcoming Sage days, depending on how good of shape my thesis is in by then).
</p>
TicketSimonKingWed, 19 May 2010 10:09:42 GMT
https://trac.sagemath.org/ticket/8800#comment:30
https://trac.sagemath.org/ticket/8800#comment:30
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:29" title="Comment 29">robertwb</a>:
</p>
<blockquote class="citation">
<p>
...
Hmm... does it make sense to have a category of Modules (over any basering)?
</p>
</blockquote>
<p>
The axioms of categories say that there must be the identity morphism for any object, and that composition of functors must be associative. It is not required that there is a morphism (e.g., the null-homomorphism) between any two objects. So, I guess a category of modules is just fine.
</p>
<p>
Cheers,
Simon
</p>
TicketSimonKingWed, 21 Jul 2010 13:31:28 GMT
https://trac.sagemath.org/ticket/8800#comment:31
https://trac.sagemath.org/ticket/8800#comment:31
<p>
There was a change needed in the patch from <a class="closed ticket" href="https://trac.sagemath.org/ticket/8807" title="enhancement: Adding support for morphisms to the category framework (closed: fixed)">#8807</a>. So, I had to rebase the ticket here. I just did!
</p>
<p>
I did not yet have the time to run <code>make ptestall</code>, but will start it right now.
</p>
TicketcremonaTue, 26 Oct 2010 20:09:26 GMTstatus changed
https://trac.sagemath.org/ticket/8800#comment:32
https://trac.sagemath.org/ticket/8800#comment:32
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
The patch here does not apply cleanly after the one at <a class="closed ticket" href="https://trac.sagemath.org/ticket/8807" title="enhancement: Adding support for morphisms to the category framework (closed: fixed)">#8807</a> (on 4.6.rc0).
</p>
TicketSimonKingTue, 26 Oct 2010 20:13:29 GMT
https://trac.sagemath.org/ticket/8800#comment:33
https://trac.sagemath.org/ticket/8800#comment:33
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:32" title="Comment 32">cremona</a>:
</p>
<blockquote class="citation">
<p>
The patch here does not apply cleanly after the one at <a class="closed ticket" href="https://trac.sagemath.org/ticket/8807" title="enhancement: Adding support for morphisms to the category framework (closed: fixed)">#8807</a> (on 4.6.rc0).
</p>
</blockquote>
<p>
Thank you for trying. Do you say that <a class="closed ticket" href="https://trac.sagemath.org/ticket/8807" title="enhancement: Adding support for morphisms to the category framework (closed: fixed)">#8807</a> did apply, but the patch here did not?
</p>
<p>
Anyway, it will take a until next week befor I will be able to resume work.
</p>
<p>
Best regards, Simon
</p>
TicketSimonKingWed, 24 Nov 2010 12:04:40 GMT
https://trac.sagemath.org/ticket/8800#comment:34
https://trac.sagemath.org/ticket/8800#comment:34
<p>
I just uploaded a new version of my patch. It does apply after the patch from <a class="closed ticket" href="https://trac.sagemath.org/ticket/8807" title="enhancement: Adding support for morphisms to the category framework (closed: fixed)">#8807</a> (with some fuzz), but now various doctests fail.
</p>
<p>
At least in one case, the reason is that some matrices still have a custom <code>__mul__</code> method were they should have a <code>_mul_</code> (single underscore) and <code>_act_on_</code> method. I expect that it will be addressed on a different ticket.
</p>
<p>
So, it needs work, but feel free to experiment with the new patch...
</p>
TicketSimonKingMon, 06 Dec 2010 10:48:31 GMTdescription, summary changed
https://trac.sagemath.org/ticket/8800#comment:35
https://trac.sagemath.org/ticket/8800#comment:35
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/8800?action=diff&version=35">diff</a>)
</li>
<li><strong>summary</strong>
changed from <em>Doctest coverage of categories</em> to <em>Doctest coverage of categories - numerous coercion fixes</em>
</li>
</ul>
TicketSimonKingMon, 06 Dec 2010 10:51:44 GMTstatus changed
https://trac.sagemath.org/ticket/8800#comment:36
https://trac.sagemath.org/ticket/8800#comment:36
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
Now, as the patch is updated, it is again ready for review. See the new ticket description for an account of what the patch does.
</p>
TicketlftaberaMon, 06 Dec 2010 11:54:29 GMTstatus changed
https://trac.sagemath.org/ticket/8800#comment:37
https://trac.sagemath.org/ticket/8800#comment:37
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
There is quite a lot of work here. Thanks!
</p>
<p>
Could you please coordinate this patch with <a class="closed ticket" href="https://trac.sagemath.org/ticket/10318" title="defect: Change CompositConstructionFunctor to CompositeConstructionFunctor (closed: fixed)">#10318</a>? That ticket already has a positive review, is related to <a class="closed ticket" href="https://trac.sagemath.org/ticket/8807" title="enhancement: Adding support for morphisms to the category framework (closed: fixed)">#8807</a> and is incompatible with <a class="closed ticket" href="https://trac.sagemath.org/ticket/8800" title="defect: Doctest coverage of categories - numerous coercion fixes (closed: fixed)">#8800</a>.
</p>
<p>
I have not read the code yet, but about problem 17. In which I participated partially I am not sure to like the solution.
</p>
<pre class="wiki">sage: K.<r4> = NumberField(x^4-2)
sage: L1.<r2_1> = NumberField(x^2-2, embedding = r4**2)
sage: L2.<r2_2> = NumberField(x^2-2, embedding = -r4**2)
sage: r2_1+r2_2 # indirect doctest
0
sage: (r2_1+r2_2).parent() is L1
True
sage: (r2_2+r2_1).parent() is L2
True
</pre><p>
Now I realise that there was some dicussion in sage-nt. Are there more examples in which the parent depends on the order of operands? I understand that this happen only where the parents are canonically isomorphic.
</p>
TicketlftaberaMon, 06 Dec 2010 12:05:02 GMT
https://trac.sagemath.org/ticket/8800#comment:38
https://trac.sagemath.org/ticket/8800#comment:38
<p>
I should have read the threads before posting. I see that the bahaviour with different parents was already present in Sage for fields with embedding to CDF. So I have nothing to say about this.
</p>
TicketSimonKingMon, 06 Dec 2010 12:35:40 GMTwork_issues set
https://trac.sagemath.org/ticket/8800#comment:39
https://trac.sagemath.org/ticket/8800#comment:39
<ul>
<li><strong>work_issues</strong>
set to <em>Compatibility with #10318</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:37" title="Comment 37">lftabera</a>:
</p>
<blockquote class="citation">
<p>
Could you please coordinate this patch with <a class="closed ticket" href="https://trac.sagemath.org/ticket/10318" title="defect: Change CompositConstructionFunctor to CompositeConstructionFunctor (closed: fixed)">#10318</a>? That ticket already has a positive review, is related to <a class="closed ticket" href="https://trac.sagemath.org/ticket/8807" title="enhancement: Adding support for morphisms to the category framework (closed: fixed)">#8807</a> and is incompatible with <a class="closed ticket" href="https://trac.sagemath.org/ticket/8800" title="defect: Doctest coverage of categories - numerous coercion fixes (closed: fixed)">#8800</a>.
</p>
</blockquote>
<p>
OK, I'll try. I'm putting it into the "work issues" field.
</p>
TicketcremonaMon, 06 Dec 2010 12:38:17 GMT
https://trac.sagemath.org/ticket/8800#comment:40
https://trac.sagemath.org/ticket/8800#comment:40
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:39" title="Comment 39">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:37" title="Comment 37">lftabera</a>:
</p>
<blockquote class="citation">
<p>
Could you please coordinate this patch with <a class="closed ticket" href="https://trac.sagemath.org/ticket/10318" title="defect: Change CompositConstructionFunctor to CompositeConstructionFunctor (closed: fixed)">#10318</a>? That ticket already has a positive review, is related to <a class="closed ticket" href="https://trac.sagemath.org/ticket/8807" title="enhancement: Adding support for morphisms to the category framework (closed: fixed)">#8807</a> and is incompatible with <a class="closed ticket" href="https://trac.sagemath.org/ticket/8800" title="defect: Doctest coverage of categories - numerous coercion fixes (closed: fixed)">#8800</a>.
</p>
</blockquote>
<p>
OK, I'll try. I'm putting it into the "work issues" field.
</p>
</blockquote>
<p>
I'm sorry that it was my trivial patch (spelling correction) which cased this. A simple search-and-replace will be all that is required to make this patch apply after <a class="closed ticket" href="https://trac.sagemath.org/ticket/10318" title="defect: Change CompositConstructionFunctor to CompositeConstructionFunctor (closed: fixed)">#10318</a>.
</p>
TicketSimonKingMon, 06 Dec 2010 13:31:40 GMTstatus changed; work_issues deleted
https://trac.sagemath.org/ticket/8800#comment:41
https://trac.sagemath.org/ticket/8800#comment:41
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>work_issues</strong>
<em>Compatibility with #10318</em> deleted
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:40" title="Comment 40">cremona</a>:
</p>
<blockquote class="citation">
<p>
I'm sorry that it was my trivial patch (spelling correction) which cased this. A simple search-and-replace will be all that is required to make this patch apply after <a class="closed ticket" href="https://trac.sagemath.org/ticket/10318" title="defect: Change CompositConstructionFunctor to CompositeConstructionFunctor (closed: fixed)">#10318</a>.
</p>
</blockquote>
<p>
Yes, it seems that it was really to solve by a simple search-and-replace. The patch should now apply after <a class="closed ticket" href="https://trac.sagemath.org/ticket/8807" title="enhancement: Adding support for morphisms to the category framework (closed: fixed)">#8807</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/10318" title="defect: Change CompositConstructionFunctor to CompositeConstructionFunctor (closed: fixed)">#10318</a>. So, back to "needs review".
</p>
<p>
Best regards,
</p>
<p>
Simon
</p>
TicketSimonKingMon, 06 Dec 2010 20:09:23 GMT
https://trac.sagemath.org/ticket/8800#comment:42
https://trac.sagemath.org/ticket/8800#comment:42
<p>
Could one of you please test on 32 bit? I had to change the doc test of selmer_group: With my patch, I get on 64-bit the same output that was previously only expected on 32-bit. So, I could imagine that the expected output on 32-bit needs to be changed as well.
</p>
<p>
Cheers,
</p>
<p>
Simon
</p>
TicketcremonaMon, 06 Dec 2010 21:12:04 GMT
https://trac.sagemath.org/ticket/8800#comment:43
https://trac.sagemath.org/ticket/8800#comment:43
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:42" title="Comment 42">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Could one of you please test on 32 bit? I had to change the doc test of selmer_group: With my patch, I get on 64-bit the same output that was previously only expected on 32-bit. So, I could imagine that the expected output on 32-bit needs to be changed as well.
</p>
</blockquote>
<p>
OK, will do -- it will on top of 4.6.1.alpha2 since I don't yet have a 32-bit build of alpha3.
</p>
<blockquote class="citation">
<p>
Cheers,
</p>
<p>
Simon
</p>
</blockquote>
TicketcremonaMon, 06 Dec 2010 21:18:35 GMT
https://trac.sagemath.org/ticket/8800#comment:44
https://trac.sagemath.org/ticket/8800#comment:44
<p>
Applies fine after <a class="closed ticket" href="https://trac.sagemath.org/ticket/8807" title="enhancement: Adding support for morphisms to the category framework (closed: fixed)">#8807</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/10318" title="defect: Change CompositConstructionFunctor to CompositeConstructionFunctor (closed: fixed)">#10318</a>. Testing now: will take some time.
</p>
<p>
I'm not sure that the buildbot will know how to apply just the last patch from <a class="closed ticket" href="https://trac.sagemath.org/ticket/8807" title="enhancement: Adding support for morphisms to the category framework (closed: fixed)">#8807</a> and then the one from <a class="closed ticket" href="https://trac.sagemath.org/ticket/10318" title="defect: Change CompositConstructionFunctor to CompositeConstructionFunctor (closed: fixed)">#10318</a>.
</p>
TicketcremonaTue, 07 Dec 2010 08:07:31 GMT
https://trac.sagemath.org/ticket/8800#comment:45
https://trac.sagemath.org/ticket/8800#comment:45
<p>
Test failures:
</p>
<pre class="wiki">sage -t "sage/groups/perm_gps/permgroup.py"
**********************************************************************
File "/home/john/sage-4.6.1.alpha2/devel/sage-main/sage/groups/perm_gps/permgroup.py", line 1114:
sage: G.random_element()
Expected:
(2,3)(4,5)
Got:
(1,2)(4,5)
**********************************************************************
1 items had failures:
1 of 4 in __main__.example_34
***Test Failed*** 1 failures.
For whitespace errors, see the file /home/john/.sage//tmp/.doctest_permgroup.py
[7.8 s]
----------------------------------------------------------------------
The following tests failed:
sage -t "sage/groups/perm_gps/permgroup.py"
Total time for all tests: 7.9 seconds
john@John-laptop%sage -t "sage/rings/number_field/number_field.py"
sage -t "sage/rings/number_field/number_field.py"
Exception RuntimeError: 'maximum recursion depth exceeded in __subclasscheck__' in <type 'exceptions.TypeError'> ignored
Exception RuntimeError: 'maximum recursion depth exceeded in __subclasscheck__' in <type 'exceptions.TypeError'> ignored
Exception RuntimeError: 'maximum recursion depth exceeded while calling a Python object' in <type 'exceptions.GeneratorExit'> ignored
Exception RuntimeError: 'maximum recursion depth exceeded while calling a Python object' in <type 'exceptions.GeneratorExit'> ignored
Exception GeneratorExit in <generator object <genexpr> at 0xc094e64> ignored
Exception RuntimeError: 'maximum recursion depth exceeded in __subclasscheck__' in <type 'exceptions.TypeError'> ignored
Exception RuntimeError: 'maximum recursion depth exceeded in __subclasscheck__' in <type 'exceptions.TypeError'> ignored
Exception RuntimeError: 'maximum recursion depth exceeded while calling a Python object' in <type 'exceptions.GeneratorExit'> ignored
Exception RuntimeError: 'maximum recursion depth exceeded while calling a Python object' in <type 'exceptions.GeneratorExit'> ignored
Exception GeneratorExit in <generator object <genexpr> at 0xc094d74> ignored
Exception RuntimeError: 'maximum recursion depth exceeded while calling a Python object' in <type 'exceptions.TypeError'> ignored
Exception RuntimeError: 'maximum recursion depth exceeded while calling a Python object' in <type 'exceptions.TypeError'> ignored
**********************************************************************
File "/home/john/sage-4.6.1.alpha2/devel/sage-main/sage/rings/number_field/number_field.py", line 2960:
sage: K.selmer_group([K.ideal(2, -a+1), K.ideal(3, a+1), K.ideal(a)], 3)
Expected:
[2, a + 1, a]
Got:
[2, a + 1, -a]
**********************************************************************
1 items had failures:
1 of 12 in __main__.example_62
***Test Failed*** 1 failures.
For whitespace errors, see the file /home/john/.sage//tmp/.doctest_number_field.py
[82.3 s]
----------------------------------------------------------------------
The following tests failed:
sage -t "sage/rings/number_field/number_field.py"
</pre>
TicketSimonKingTue, 07 Dec 2010 08:22:26 GMT
https://trac.sagemath.org/ticket/8800#comment:46
https://trac.sagemath.org/ticket/8800#comment:46
<p>
Hi John!
</p>
<p>
Is that 32 or 64 bit? Because:
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:45" title="Comment 45">cremona</a>:
</p>
<blockquote class="citation">
<p>
sage -t "sage/groups/perm_gps/permgroup.py"
<strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong>
File "/home/john/sage-4.6.1.alpha2/devel/sage-main/sage/groups/perm_gps/permgroup.py", line 1114:
</strong></p>
<blockquote>
<p>
sage: G.random_element()
</p>
</blockquote>
<p>
Expected:
</p>
<blockquote>
<p>
(2,3)(4,5)
</p>
</blockquote>
<p>
Got:
</p>
<blockquote>
<p>
(1,2)(4,5)
</p>
</blockquote>
</blockquote>
<p>
I changed the expected element. (1,2)(4,5) was originally expected, but I obtain (2,3)(4,5) on my machine (after applying the patch).
</p>
<blockquote class="citation">
<p>
File "/home/john/sage-4.6.1.alpha2/devel/sage-main/sage/rings/number_field/number_field.py", line 2960:
</p>
<blockquote>
<p>
sage: K.selmer_group([K.ideal(2, -a+1), K.ideal(3, a+1), K.ideal(a)], 3)
</p>
</blockquote>
<p>
Expected:
</p>
<blockquote>
<p>
[2, a + 1, a]
</p>
</blockquote>
<p>
Got:
</p>
<blockquote>
<p>
[2, a + 1, -a]
</p>
</blockquote>
</blockquote>
<p>
This one I also changed. [2, a + 1, -a] was originally expected with 64-bit. But after applying the patch, I got [2, a + 1, a], which was originally expected with 32-bit.
</p>
<p>
Strange. What can one do to get a reproducible result?
</p>
TicketlftaberaTue, 07 Dec 2010 08:56:50 GMT
https://trac.sagemath.org/ticket/8800#comment:47
https://trac.sagemath.org/ticket/8800#comment:47
<p>
I got the same error in permgroup.py in both 32 and 64 bits. I got the permutation (1,2)(4,5) in two different machines with 4.6 + patches from this ticket.
</p>
<p>
We can investigate further what is going on, but I do not like this kind of tests against random_element. Even if we use the same seed. Is there a policy to deal with random_element methods?
</p>
<p>
What about something like?
</p>
<pre class="wiki">sage: a= G.random_element()
sage: a in G
True
sage: a.parent() is G
True
sage: a**6
()
</pre><p>
About the errors in number_field all tests passes in 64 bits but I get the same errors as John in 32 bits. Concerning selmer group. Are both results right or only one of them?
</p>
TicketSimonKingTue, 07 Dec 2010 09:08:26 GMT
https://trac.sagemath.org/ticket/8800#comment:48
https://trac.sagemath.org/ticket/8800#comment:48
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:47" title="Comment 47">lftabera</a>:
</p>
<blockquote class="citation">
<p>
I got the same error in permgroup.py in both 32 and 64 bits. I got the permutation (1,2)(4,5) in two different machines with 4.6 + patches from this ticket.
</p>
</blockquote>
<p>
Really strange.
</p>
<blockquote class="citation">
<p>
We can investigate further what is going on, but I do not like this kind of tests against random_element. Even if we use the same seed.
</p>
</blockquote>
<p>
Well, we <em>do</em> use the same seed. So, it has to be reproducible.
</p>
<blockquote class="citation">
<p>
What about something like?
</p>
<pre class="wiki">sage: a= G.random_element()
sage: a in G
True
sage: a.parent() is G
True
sage: a**6
()
</pre></blockquote>
<p>
I guess there is currently a related discussion at <a class="ext-link" href="http://groups.google.com/group/sage-devel/browse_thread/thread/c201930abdbd23d3"><span class="icon"></span>sage-devel</a>
</p>
<blockquote class="citation">
<p>
About the errors in number_field all tests passes in 64 bits but I get the same errors as John in 32 bits. Concerning selmer group. Are both results right or only one of them?
</p>
</blockquote>
<p>
Both are right. The method is supposed to return <em>a</em> generating set. And that is the case for both answers. And in the original version, the expected answer did depend on 32- versus 64-bit.
</p>
<p>
Best regards,
</p>
<p>
Simon
</p>
TicketcremonaTue, 07 Dec 2010 09:11:55 GMT
https://trac.sagemath.org/ticket/8800#comment:49
https://trac.sagemath.org/ticket/8800#comment:49
<p>
My tests were on 32-bit 4.6.1.alpha2.
</p>
<p>
In the Selmer group test both results are correct. It is very common for pari output to be different on 32- and 64-bit, and that the underlying this computation. The output numbers are generating a group which is abstractly (Z/3Z)3, so there is no unique generating set; and (worse) the elements themselves are representatives of cosets of K*/(K*)3.
</p>
TicketSimonKingTue, 07 Dec 2010 09:24:44 GMT
https://trac.sagemath.org/ticket/8800#comment:50
https://trac.sagemath.org/ticket/8800#comment:50
<p>
Hi John!
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:49" title="Comment 49">cremona</a>:
</p>
<blockquote class="citation">
<p>
My tests were on 32-bit 4.6.1.alpha2.
</p>
</blockquote>
<p>
OK, that means that the results on 32-bit and on 64-bit are switched: I get on 64-bit the result that was previously expected on 32-bit, and you get on 32-bit the result that was previously expected on 64-bit. Or am I confusing things?
</p>
TicketcremonaTue, 07 Dec 2010 09:37:04 GMT
https://trac.sagemath.org/ticket/8800#comment:51
https://trac.sagemath.org/ticket/8800#comment:51
<p>
It is surely possible that there are other differences between alpha2 and alpha3, so perhaps I should test again when I have built alpha3. I just started that. (This is a different machine -- my desktop at work -- than the one I tested alpha2 on, which was my laptop at home).
</p>
TicketSimonKingTue, 07 Dec 2010 10:15:58 GMT
https://trac.sagemath.org/ticket/8800#comment:52
https://trac.sagemath.org/ticket/8800#comment:52
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:51" title="Comment 51">cremona</a>:
</p>
<blockquote class="citation">
<p>
It is surely possible that there are other differences between alpha2 and alpha3, so perhaps I should test again when I have built alpha3. I just started that. (This is a different machine -- my desktop at work -- than the one I tested alpha2 on, which was my laptop at home).
</p>
</blockquote>
<p>
Even more confusing...
</p>
<p>
By the way, I tested based on sage-4.6, so, no alpha version.
</p>
TicketcremonaTue, 07 Dec 2010 14:36:06 GMTstatus changed
https://trac.sagemath.org/ticket/8800#comment:53
https://trac.sagemath.org/ticket/8800#comment:53
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
I just tested on a different 32-bit machine on which I have just built 4.6.1.alpha3 (and all tests passed): Same failure as before for sage/groups/perm_gps/permgroup.py, and for sage/groups/perm_gps/permgroup.py
</p>
<p>
The second one of these is the more worrying in that it goes into an infinite recursion.
</p>
TicketSimonKingTue, 07 Dec 2010 16:23:25 GMT
https://trac.sagemath.org/ticket/8800#comment:54
https://trac.sagemath.org/ticket/8800#comment:54
<p>
Hi John,
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:53" title="Comment 53">cremona</a>:
</p>
<blockquote class="citation">
<p>
I just tested on a different 32-bit machine on which I have just built 4.6.1.alpha3 (and all tests passed): Same failure as before for sage/groups/perm_gps/permgroup.py, and for sage/groups/perm_gps/permgroup.py
</p>
<p>
The second one of these is the more worrying in that it goes into an infinite recursion.
</p>
</blockquote>
<p>
I wonder if the recursion comes from the testing framework. Once, I observed such recursion in a test, but I could not reproduce it in an interactive session. In addition, I got a return value different from the expected - and when I changed the expected value in the test, the recursion disappeared as well.
</p>
<p>
Could you change the expected 32-bit value in the test of selmer_group to the value that you get in an interactive session, and then try <code>sage -t "sage/rings/number_field/number_field.py"</code> again?
</p>
<p>
Cheers,
</p>
<p>
Simon
</p>
TicketcremonaTue, 07 Dec 2010 18:18:22 GMT
https://trac.sagemath.org/ticket/8800#comment:55
https://trac.sagemath.org/ticket/8800#comment:55
<p>
OK, I tried that. Now all tests pass. The relevant lines now look like
</p>
<pre class="wiki"> sage: K.selmer_group([K.ideal(2, -a+1), K.ideal(3, a+1), K.ideal(a)], 3)
[2, a + 1, -a] # 32-bit
[2, a + 1, a] # 64-bit
</pre><p>
while before the two expected outputs were the same despite the separation into 32 and 64 bit cases. Was this just a typo?
</p>
<p>
There is still no explanation for why, when the expected and actual output differed, there was that infinite recursion.
</p>
TicketSimonKingTue, 07 Dec 2010 18:46:20 GMT
https://trac.sagemath.org/ticket/8800#comment:56
https://trac.sagemath.org/ticket/8800#comment:56
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:55" title="Comment 55">cremona</a>:
</p>
<blockquote class="citation">
<p>
while before the two expected outputs were the same despite the separation into 32 and 64 bit cases. Was this just a typo?
</p>
</blockquote>
<p>
No. I simply have no 32-bit machine and couldn't test it. That's why I asked that some of you please test on 32-bit.
</p>
<blockquote class="citation">
<p>
There is still no explanation for why, when the expected and actual output differed, there was that infinite recursion.
</p>
</blockquote>
<p>
Yes. But it seems to me that it is located in the doctest framework.
</p>
<p>
So, unless you find more issues, I will post another patch that changes the expected value in case of 32-bit.
</p>
<p>
Cheers,
</p>
<p>
Simon
</p>
TicketcremonaTue, 07 Dec 2010 19:41:09 GMT
https://trac.sagemath.org/ticket/8800#comment:57
https://trac.sagemath.org/ticket/8800#comment:57
<blockquote class="citation">
<p>
So, unless you find more issues, I will post another patch that changes the expected value in case of 32-bit.
</p>
</blockquote>
<p>
Go for it!
</p>
TicketlftaberaTue, 07 Dec 2010 20:00:33 GMT
https://trac.sagemath.org/ticket/8800#comment:58
https://trac.sagemath.org/ticket/8800#comment:58
<p>
I confirm that changing the doctest makes all doctest pass.
</p>
<p>
However, with the coercion of embedded and non embedded number fields, now addition is not associative.
</p>
<pre class="wiki">sage: K1.<r1>=NumberField(x^2-2)
sage: K2.<r2>=NumberField(x^2-2, embedding=1)
sage: K3.<r3>=NumberField(x^2-2, embedding=-1)
sage: (r1+r2)+r3
3*r1
sage: r1+(r2+r3)
r1
</pre><p>
r1+r2 is ambiguous. So either this operation should raise an error or it should add an embedding to K1 compatible with K2. But as far as I understand the coercion model the latter is not possible.
</p>
TicketSimonKingTue, 07 Dec 2010 20:31:49 GMT
https://trac.sagemath.org/ticket/8800#comment:59
https://trac.sagemath.org/ticket/8800#comment:59
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:58" title="Comment 58">lftabera</a>:
</p>
<blockquote class="citation">
<p>
I confirm that changing the doctest makes all doctest pass.
</p>
</blockquote>
<p>
Good!
</p>
<blockquote class="citation">
<p>
However, with the coercion of embedded and non embedded number fields, now addition is not associative.
</p>
<pre class="wiki">sage: K1.<r1>=NumberField(x^2-2)
sage: K2.<r2>=NumberField(x^2-2, embedding=1)
sage: K3.<r3>=NumberField(x^2-2, embedding=-1)
sage: (r1+r2)+r3
3*r1
sage: r1+(r2+r3)
r1
</pre><p>
r1+r2 is ambiguous. So either this operation should raise an error or it should add an embedding to K1 compatible with K2. But as far as I understand the coercion model the latter is not possible.
</p>
</blockquote>
<p>
I disagree: It should not raise an error. This is a side-effect of Sage's coercion model. We (see discussion on sage-nt) do want a forgetful coercion from K2 to K1 and from K3 to K1; and we want a coercion between two embedded number fields induced by the embedding.
</p>
<p>
Hence, we have a coercion between K2 and K3 sending <code>r3</code> to <code>-r2</code>. Therefore <code>r2+r3</code> is <code>K2.zero()</code>, thus, <code>r1+(r2+r3)==r1</code>. On the other hand, <code>r1+r2</code> is <code>2*r1</code>, since the coercion from K2 to K1 sends <code>r2</code> to <code>r1</code>; and similarly <code>r3</code> is sent to <code>r1</code>, hence <code>(r1+r2)+r3==3*r1</code>.
</p>
<p>
But I suggest to discuss on sage-algebra whether people are really happy with that consequence of a forgetful coercion.
</p>
TicketSimonKingWed, 08 Dec 2010 06:53:49 GMTwork_issues set
https://trac.sagemath.org/ticket/8800#comment:60
https://trac.sagemath.org/ticket/8800#comment:60
<ul>
<li><strong>work_issues</strong>
set to <em>change 32-bit test; remove forgetful coercion</em>
</li>
</ul>
TicketSimonKingWed, 08 Dec 2010 07:00:40 GMT
https://trac.sagemath.org/ticket/8800#comment:61
https://trac.sagemath.org/ticket/8800#comment:61
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:58" title="Comment 58">lftabera</a>:
</p>
<blockquote class="citation">
<p>
However, with the coercion of embedded and non embedded number fields, now addition is not associative.
</p>
</blockquote>
<p>
As you (? I guess <code>luisfe == lftabera</code>) pointed out at <a class="ext-link" href="http://groups.google.com/group/sage-algebra/browse_thread/thread/889464bee6a6a036"><span class="icon"></span>sage-algebra</a>, the actual problem is not the non-associativity of the addition (after all, we have different algebraic structures involved, so, there is no reason to expect that it can be globally extended to something that is associative).
</p>
<p>
You convinced me that the actual problem is the fact that the coercions in your example do not form a commuting triangle: Coercion from <code>K3</code> to <code>K2</code> followed by forgetful coercion from <code>K2</code> to <code>K1</code> is not the same as the forgetful coercion from <code>K3</code> to <code>K1</code>.
</p>
<p>
Hence, I have to modify the <code>_coerce_map_from_</code> of number fields and probably also the merge method of <code>AlgebraicExtensionFunctor</code>.
</p>
TicketSimonKingWed, 08 Dec 2010 09:01:10 GMTdescription changed
https://trac.sagemath.org/ticket/8800#comment:62
https://trac.sagemath.org/ticket/8800#comment:62
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/8800?action=diff&version=62">diff</a>)
</li>
</ul>
TicketSimonKingWed, 08 Dec 2010 09:04:25 GMTstatus changed; work_issues deleted
https://trac.sagemath.org/ticket/8800#comment:63
https://trac.sagemath.org/ticket/8800#comment:63
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>work_issues</strong>
<em>change 32-bit test; remove forgetful coercion</em> deleted
</li>
</ul>
<p>
I removed the "forgetful coercions" and changed the documentation and the ticket description accordingly.
</p>
<p>
I modified the 32-bit expected value of selmer_group according to your findings (but please test if it really works 32-bit; I only tested 64-bit).
</p>
<p>
I modified the annoying random_element test in permgroup.py as Luis suggested.
</p>
<p>
Hence, I think it is ready for review again!
</p>
TicketSimonKingWed, 08 Dec 2010 10:28:13 GMT
https://trac.sagemath.org/ticket/8800#comment:64
https://trac.sagemath.org/ticket/8800#comment:64
<p>
depends on <a class="closed ticket" href="https://trac.sagemath.org/ticket/8807" title="enhancement: Adding support for morphisms to the category framework (closed: fixed)">#8807</a> <a class="closed ticket" href="https://trac.sagemath.org/ticket/10318" title="defect: Change CompositConstructionFunctor to CompositeConstructionFunctor (closed: fixed)">#10318</a>
</p>
TicketlftaberaThu, 09 Dec 2010 09:34:55 GMT
https://trac.sagemath.org/ticket/8800#comment:65
https://trac.sagemath.org/ticket/8800#comment:65
<p>
I confirm that the new patch applies to 32-bits linux and long doctest passes. I have not read the code yet.
</p>
TicketSimonKingWed, 29 Dec 2010 20:28:16 GMT
https://trac.sagemath.org/ticket/8800#comment:66
https://trac.sagemath.org/ticket/8800#comment:66
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:65" title="Comment 65">lftabera</a>:
</p>
<blockquote class="citation">
<p>
I confirm that the new patch applies to 32-bits linux and long doctest passes. I have not read the code yet.
</p>
</blockquote>
<p>
Could the doctests be repeated, please? I just did it on my machine, based on <code>sage-4.6.1.alpha3</code>, and again the problem is the doctest for <code>selmer_group</code>. Recall that in the original patch I had to switch the expected values for 32- and 64-bit. And now, with <code>sage-4.6.1.alpha3</code>, I get again the value that was originally expected without the patch.
</p>
<p>
Therefore I'll replace the patch again, in a few minutes. Please test!
</p>
TicketSimonKingSat, 29 Jan 2011 09:11:20 GMT
https://trac.sagemath.org/ticket/8800#comment:67
https://trac.sagemath.org/ticket/8800#comment:67
<p>
I really don't see what the patchbot was complaining about. The old patch did apply to <code>sage-4.6.2.alpha0</code> with just a little fuzz.
</p>
<p>
Anyway, I refreshed it. The dependencies of the ticket are already merged in <code>sage-4.6.2.alpha0</code>, so, it should now apply cleanly.
</p>
<p>
Please, try to review it! I really think that fixing so many bugs and providing full doctest coverage of a large chunk of the coercion machinery is worth the effort.
</p>
TicketlftaberaMon, 14 Feb 2011 14:49:27 GMTstatus changed
https://trac.sagemath.org/ticket/8800#comment:68
https://trac.sagemath.org/ticket/8800#comment:68
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Hi Simon,
</p>
<p>
I am reading the code, it is a long patch but looks good, thanks for the work done.
</p>
<p>
I have a question about functor <a class="missing wiki">AlgebraicExtensionFunctor?</a> and ZZ. According to the documentation:
</p>
<p>
When applying a number field constructor to the ring of integers, the maximal order in the number field is returned::
</p>
<p>
Why is this chosen instead of ZZ[x]/polynomial?
</p>
<p>
Actually, the code does not follow the documentation except for <a class="missing wiki">CyclotomicField?</a>:
</p>
<pre class="wiki">sage: N = NumberField(x^2 - 5, 'a')
sage: F, R = N.construction()
sage: F(ZZ).gens()
[1, a]
sage: F(ZZ).is_maximal()
False
sage: N.maximal_order().gens()
[1/2*a + 1/2, a]
</pre><p>
I add a patch that contains some small improvements (in my opinion). A couple of small tests and some style. Plase consider merging some of these changes. For example, in the code you usually write:
</p>
<p>
return
</p>
<p>
instead of
</p>
<p>
return None
</p>
<p>
Both are correct but, unless there are other reasons I am unaware, the second looks more readable to me (just an opinion).
</p>
<p>
I have not yet finish to review the whole patch, so you may consider waiting untill I am done. I have to compile the documentation and check that the list of bugs you have solved appears in the TESTS of the patch.
</p>
TicketSimonKingMon, 14 Feb 2011 15:13:20 GMT
https://trac.sagemath.org/ticket/8800#comment:69
https://trac.sagemath.org/ticket/8800#comment:69
<p>
Hi Luis,
</p>
<p>
First of all, thank you for looking at the patch and finding so many typos.
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:68" title="Comment 68">lftabera</a>:
</p>
<blockquote class="citation">
<p>
I have a question about functor <a class="missing wiki">AlgebraicExtensionFunctor?</a> and ZZ. According to the documentation:
</p>
<p>
When applying a number field constructor to the ring of integers, the maximal order in the number field is returned::
</p>
<p>
Why is this chosen instead of ZZ[x]/polynomial?
</p>
</blockquote>
<p>
That is how currently extensions of <code>ZZ</code> behave:
</p>
<pre class="wiki">sage: ZZ.extension(x^2+3*x+1,names=['y'])
Order in Number Field in y with defining polynomial x^2 + 3*x + 1
</pre><p>
So, it wasn't my idea; the construction functor is merely mimicking what the <code>extension</code> method of <code>ZZ</code> was doing anyway.
</p>
<blockquote class="citation">
<p>
Actually, the code does not follow the documentation except for <a class="missing wiki">CyclotomicField?</a>:
</p>
<pre class="wiki">sage: N = NumberField(x^2 - 5, 'a')
sage: F, R = N.construction()
sage: F(ZZ).gens()
[1, a]
sage: F(ZZ).is_maximal()
False
sage: N.maximal_order().gens()
[1/2*a + 1/2, a]
</pre></blockquote>
<p>
Again, this is what <code>ZZ.extension</code> currently does:
</p>
<pre class="wiki">sage: ZZ.extension(x^2 - 5, 'a').is_maximal()
False
</pre><p>
But I don't understand why that contradicts the documentation? Is it since I wrote "Note that the construction functor of a number field returns the order of that field"? <em>The</em> order?
</p>
<p>
Perhaps I should better write "Note that the construction functor of a number field applied to the integers returns an order of that field, similar to the behaviour of <code>ZZ.extension</code>"?
</p>
<blockquote class="citation">
<p>
I add a patch that contains some small improvements (in my opinion). A couple of small tests and some style. Plase consider merging some of these changes.
</p>
</blockquote>
<p>
I agree with all changes that you suggest in your "some_ideas" patch - so, once you're done, please promote it to a referee patch!
</p>
<p>
Best regards,
</p>
<p>
Simon
</p>
TicketlftaberaMon, 14 Feb 2011 15:28:55 GMT
https://trac.sagemath.org/ticket/8800#comment:70
https://trac.sagemath.org/ticket/8800#comment:70
<p>
Ok, current behaviur is what I would expect. But then there is a typo in
<a class="missing wiki">AlgebraicExtensionFunctor?</a>.<span class="underline">init</span> which is where the documentation claims that returs the maximal order. Lines 2223 and 2224 of your patch:
</p>
<pre class="wiki">+ When applying a number field constructor to the ring of integers,
+ the maximal order in the number field is returned::
</pre>
TicketSimonKingMon, 14 Feb 2011 15:37:47 GMT
https://trac.sagemath.org/ticket/8800#comment:71
https://trac.sagemath.org/ticket/8800#comment:71
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:70" title="Comment 70">lftabera</a>:
</p>
<blockquote class="citation">
<p>
Ok, current behaviur is what I would expect. But then there is a typo in
<a class="missing wiki">AlgebraicExtensionFunctor?</a>.<span class="underline">init</span> which is where the documentation claims that returs the maximal order. Lines 2223 and 2224 of your patch:
</p>
</blockquote>
<p>
Thanks! That ought to change, then. I only found the other place, where I wrote "the order" rather than "an order".
</p>
TicketSimonKingMon, 14 Feb 2011 16:44:44 GMTstatus changed
https://trac.sagemath.org/ticket/8800#comment:72
https://trac.sagemath.org/ticket/8800#comment:72
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
Dear Luis,
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:71" title="Comment 71">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:70" title="Comment 70">lftabera</a>:
</p>
<blockquote class="citation">
<p>
Ok, current behaviur is what I would expect. But then there is a typo in
<a class="missing wiki">AlgebraicExtensionFunctor?</a>.<span class="underline">init</span> which is where the documentation claims that returs the maximal order. Lines 2223 and 2224 of your patch:
</p>
</blockquote>
<p>
Thanks! That ought to change, then. I only found the other place, where I wrote "the order" rather than "an order".
</p>
</blockquote>
<p>
This is now fixed.
</p>
<p>
I change the ticket status into "needs review" again, since I believe the other typos can be fixed with your reviewer patch.
</p>
TicketlftaberaWed, 16 Feb 2011 18:01:17 GMTstatus changed
https://trac.sagemath.org/ticket/8800#comment:73
https://trac.sagemath.org/ticket/8800#comment:73
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_info</em>
</li>
</ul>
<p>
Simon,
</p>
<p>
What is the reason for the following change?
</p>
<pre class="wiki">diff -r f71dd979f978 -r 7097db76160e sage/rings/rational_field.py
--- a/sage/rings/rational_field.py Fri Dec 10 14:50:18 2010 +0100
+++ b/sage/rings/rational_field.py Wed Jul 21 14:25:41 2010 +0100
@@ -253,7 +253,7 @@
import integer_ring
return FractionField(), integer_ring.ZZ
- def completion(self, p, prec, extras = {}):
+ def completion(self, p, prec, extras):
</pre><p>
In the completion method of the <a class="missing wiki">RationalField?</a>. I think it is an error to eliminate the default extras = {}. It is not a mandatory argument neither for Qp not for create_RealField and the user has no idea of what to put there (QQ.completion has no documentation, which is a bug, but not for this ticket)
</p>
<p>
Luis
</p>
TicketSimonKingWed, 16 Feb 2011 18:13:58 GMT
https://trac.sagemath.org/ticket/8800#comment:74
https://trac.sagemath.org/ticket/8800#comment:74
<p>
Hi Luis,
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:73" title="Comment 73">lftabera</a>:
</p>
<blockquote class="citation">
<blockquote>
<p>
What is the reason for the following change?
</p>
</blockquote>
<p>
...
</p>
<blockquote>
<p>
In the completion method of the <a class="missing wiki">RationalField?</a>. I think it is an error to eliminate the default extras = {}.
</p>
</blockquote>
</blockquote>
<p>
I have not the faintest idea why I did that change. Probably it was by accident. I'll try to revert that change and see if tests still pass.
</p>
<p>
Cheers, Simon
</p>
TicketSimonKingWed, 16 Feb 2011 20:45:18 GMT
https://trac.sagemath.org/ticket/8800#comment:75
https://trac.sagemath.org/ticket/8800#comment:75
<p>
Apply 8800_functor_pushout_doc_and_fixes.patch some_ideas.patch
</p>
<p>
(For the patchbot, if that's necessary)
</p>
TicketSimonKingWed, 16 Feb 2011 20:49:17 GMTstatus changed
https://trac.sagemath.org/ticket/8800#comment:76
https://trac.sagemath.org/ticket/8800#comment:76
<ul>
<li><strong>status</strong>
changed from <em>needs_info</em> to <em>needs_review</em>
</li>
</ul>
<p>
I reverted the obscure "extras" issue. That was no problem.
</p>
<p>
I had to change two doctests that used the ".transpose()" method for vectors, which is now deprecated. With the new patch applied to sage-4.6.2.alpha4, the long doctests pass on my machine.
</p>
<p>
Presumably, your "some_ideas.patch" will become a reviewer patch, thus I told the patchbot to apply it.
</p>
<p>
Best regards,
</p>
<p>
Simon
</p>
TicketSimonKingThu, 17 Feb 2011 07:30:18 GMT
https://trac.sagemath.org/ticket/8800#comment:77
https://trac.sagemath.org/ticket/8800#comment:77
<p>
FWIW, the doctest failure reported by the patchbot comes from the fact that the vector method ".column()" seems to be a feature only introduced in 4.6.2.alpha2, replacing the now deprecated ".transpose()".
</p>
TicketlftaberaFri, 18 Feb 2011 16:39:38 GMTattachment set
https://trac.sagemath.org/ticket/8800
https://trac.sagemath.org/ticket/8800
<ul>
<li><strong>attachment</strong>
set to <em>some_ideas.patch</em>
</li>
</ul>
<p>
Ideas to consider merging
</p>
TicketlftaberaFri, 18 Feb 2011 16:44:13 GMTstatus, milestone changed
https://trac.sagemath.org/ticket/8800#comment:78
https://trac.sagemath.org/ticket/8800#comment:78
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>milestone</strong>
changed from <em>sage-4.6.2</em> to <em>sage-4.7</em>
</li>
</ul>
<p>
The documentation of pushout is not built in the reference manual. I have added pushout.py to categories.rst, but I get warnings and errors in the html and pdf built that I do not know how to solve.
</p>
TicketSimonKingFri, 18 Feb 2011 20:13:46 GMT
https://trac.sagemath.org/ticket/8800#comment:79
https://trac.sagemath.org/ticket/8800#comment:79
<p>
I just updated my patch, also merging your some_ideas.patch. The documentation seemed to build without problems (which required some editing). I am afraid I will probably be unable to see the documentation for the next ten days, as I will not be in my office.
</p>
<p>
But then I made a big mistake: I also included an autogenerated file into the repository, namely doc/en/reference/sage/categories/pushout.rst. When I noticed it and tried to <code>hg delete</code> it, apparently I managed to kill the entire documentation. I don't know whether I will recover from that stroke, because even <code>sage -docbuild reference html</code> did not help.
</p>
<p>
But perhaps you will be able to (1) see whether the documentation of sage.categories.pushout looks nice and (2) correct my patch?
</p>
<p>
Apply 8800_functor_pushout_doc_and_fixes.patch
</p>
TicketSimonKingSat, 19 Feb 2011 08:27:31 GMTstatus changed
https://trac.sagemath.org/ticket/8800#comment:80
https://trac.sagemath.org/ticket/8800#comment:80
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
I think I solved the trouble with the reference manual. The new patch includes the some_ideas patch. So, only one patch needs to be applied.
</p>
<p>
With the patch, the references for sage.categories.pushout build, and no warning or error is raised. But I am currently not able to watch the result (this will take more than one week). So, I ask the reviewer to have a look on it.
</p>
<p>
To be on the safe side, I am now running doc tests (at least, <code>sage -tp 4 doc/en</code> passes). But I think I can revert it to "needs review".
</p>
TicketSimonKingSat, 19 Feb 2011 08:29:39 GMT
https://trac.sagemath.org/ticket/8800#comment:81
https://trac.sagemath.org/ticket/8800#comment:81
<p>
Apply 8800_functor_pushout_doc_and_fixes.patch
</p>
<p>
(for the patchbot)
</p>
TicketSimonKingSat, 19 Feb 2011 16:19:36 GMT
https://trac.sagemath.org/ticket/8800#comment:82
https://trac.sagemath.org/ticket/8800#comment:82
<p>
All long tests pass if one applies the patch to sage-4.6.2.alpha4. The patchbot uses sage-4.6.1, that's why it finds two errors.
</p>
TicketlftaberaSat, 26 Feb 2011 11:21:37 GMTattachment set
https://trac.sagemath.org/ticket/8800
https://trac.sagemath.org/ticket/8800
<ul>
<li><strong>attachment</strong>
set to <em>referee.patch</em>
</li>
</ul>
TicketlftaberaSat, 26 Feb 2011 11:29:16 GMTdescription changed; reviewer set
https://trac.sagemath.org/ticket/8800#comment:83
https://trac.sagemath.org/ticket/8800#comment:83
<ul>
<li><strong>reviewer</strong>
set to <em>Luis Felipe Tabera Alonso</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/8800?action=diff&version=83">diff</a>)
</li>
</ul>
<p>
I have tested with sage-4.6.1.rc0 the documentation builds and looks good. The bugs have been corrected and the patch introduces some very nice features. Good work. Positive review to Simon's patch.
</p>
<p>
However, I have added a referee patch with some minor changes in the documentation. I have eliminated some latex code that, in my opinion, made the documentation harder to read.
</p>
<p>
Simon, could you look at my patch? If you feel it is ok, put a positive review.
</p>
TicketSimonKingSat, 26 Feb 2011 12:55:12 GMTstatus changed
https://trac.sagemath.org/ticket/8800#comment:84
https://trac.sagemath.org/ticket/8800#comment:84
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8800#comment:83" title="Comment 83">lftabera</a>:
</p>
<blockquote class="citation">
<p>
However, I have added a referee patch with some minor changes in the documentation. I have eliminated some latex code that, in my opinion, made the documentation harder to read.
</p>
<p>
Simon, could you look at my patch? If you feel it is ok, put a positive review.
</p>
</blockquote>
<p>
I have read the referee patch, and it seems fine. So, I guess it is a positive review then. Finally!
</p>
<p>
Thank you,
</p>
<p>
Simon
</p>
TicketjdemeyerMon, 28 Feb 2011 14:07:21 GMTstatus changed; work_issues set
https://trac.sagemath.org/ticket/8800#comment:85
https://trac.sagemath.org/ticket/8800#comment:85
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
<li><strong>work_issues</strong>
set to <em>rebase</em>
</li>
</ul>
<p>
This should be rebased to sage-4.6.2.rc1 + <a class="closed ticket" href="https://trac.sagemath.org/ticket/10677" title="enhancement: Improve PARI interface for relative number fields (closed: fixed)">#10677</a> + <a class="closed ticket" href="https://trac.sagemath.org/ticket/2329" title="enhancement: Add interface to PARI's rnfisnorm() (closed: fixed)">#2329</a> (or, you can wait until sage-4.7.alpha1 is released and then rebase to that).
</p>
TicketSimonKingTue, 08 Mar 2011 14:18:23 GMTattachment set
https://trac.sagemath.org/ticket/8800
https://trac.sagemath.org/ticket/8800
<ul>
<li><strong>attachment</strong>
set to <em>8800_functor_pushout_doc_and_fixes.patch</em>
</li>
</ul>
<p>
Full doctest coverage for sage.categories.functor and sage.categories.pushout. Various coercion bug fixes.
</p>
TicketSimonKingTue, 08 Mar 2011 14:20:11 GMTstatus changed; work_issues deleted
https://trac.sagemath.org/ticket/8800#comment:86
https://trac.sagemath.org/ticket/8800#comment:86
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>work_issues</strong>
<em>rebase</em> deleted
</li>
</ul>
<p>
Depends on <a class="closed ticket" href="https://trac.sagemath.org/ticket/10677" title="enhancement: Improve PARI interface for relative number fields (closed: fixed)">#10677</a> <a class="closed ticket" href="https://trac.sagemath.org/ticket/2329" title="enhancement: Add interface to PARI's rnfisnorm() (closed: fixed)">#2329</a>
</p>
<p>
Apply 8800_functor_pushout_doc_and_fixes.patch referee.patch
</p>
TicketSimonKingTue, 08 Mar 2011 14:22:25 GMT
https://trac.sagemath.org/ticket/8800#comment:87
https://trac.sagemath.org/ticket/8800#comment:87
<p>
I was rebasing the main patch, so that it applies cleanly on top of sage-4.6.2 plus <a class="closed ticket" href="https://trac.sagemath.org/ticket/10677" title="enhancement: Improve PARI interface for relative number fields (closed: fixed)">#10677</a> plus <a class="closed ticket" href="https://trac.sagemath.org/ticket/2329" title="enhancement: Add interface to PARI's rnfisnorm() (closed: fixed)">#2329</a>. I change it into "needs review", since I am now running doctests.
</p>
<p>
I hope I am entitled to revert to the old positive review, provided that the long tests pass.
</p>
TicketjdemeyerTue, 08 Mar 2011 14:26:57 GMTdescription changed
https://trac.sagemath.org/ticket/8800#comment:88
https://trac.sagemath.org/ticket/8800#comment:88
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/8800?action=diff&version=88">diff</a>)
</li>
</ul>
TicketSimonKingTue, 08 Mar 2011 15:10:48 GMTstatus changed
https://trac.sagemath.org/ticket/8800#comment:89
https://trac.sagemath.org/ticket/8800#comment:89
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
I have absolutely no idea what the patchbot is complaining about! Its shortlog states
</p>
<pre class="wiki">2011-03-08 06:20:58 -0800
None
2011-03-08 06:21:05 -0800
7 seconds
</pre><p>
which means???
</p>
<p>
Anyway. All long tests both in <code>sage/</code> and in <code>doc/</code> pass. So, if nobody objects, I return to the old positive review.
</p>
TicketjdemeyerTue, 08 Mar 2011 21:45:09 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/8800#comment:90
https://trac.sagemath.org/ticket/8800#comment:90
<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>merged</strong>
set to <em>sage-4.7.alpha1</em>
</li>
</ul>
Ticket