Sage: Ticket #9054: create a class for basic function_field arithmetic for Sage
https://trac.sagemath.org/ticket/9054
<p>
One of the first things we learned at Sage Days 21: Function Fields, is that it is not even possible to really define or even do arithmetic in function fields *at all* in Sage! It's amazing that this most basic arithmetic still isn't supported, but it isn't (maybe it used to be via generic machinery, but got broken...?). The point of this ticket is to create classes for standard function field structures, along with support for arithmetic. This should be organized in a way similar to number fields.
</p>
<p>
For this code, the main point is to establish an API that works solidly. It will be insanely slow. A subsequent patch will make things fast.
</p>
<p>
See also: <a class="closed ticket" href="https://trac.sagemath.org/ticket/9069" title="defect: weak popov form (closed: fixed)">#9069</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/9051" title="defect: Fast function field arithmetic (closed: fixed)">#9051</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/9094" title="enhancement: is_square and sqrt for polynomials and fraction fields (closed: fixed)">#9094</a>, <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/9095" title="enhancement: Elliptic curve over function field heights (needs_work)">#9095</a>.
</p>
<p>
Note that the dependancy on <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a> is only because of a really minor change in the doctests. This ticket already has a positive review so I suspect this will get merged first. If that ticket eventually gets rejected it will be trivial to rebase the patch withouth that ticket.
</p>
<p>
Apply <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9054/9054_function_fields.patch" title="Attachment '9054_function_fields.patch' in Ticket #9054">9054_function_fields.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9054/9054_function_fields.patch" title="Download"></a> to the Sage library.
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/9054
Trac 1.1.6wasWed, 26 May 2010 11:32:21 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054-part1.patch</em>
</li>
</ul>
TicketburcinWed, 26 May 2010 22:22:52 GMTcc set
https://trac.sagemath.org/ticket/9054#comment:1
https://trac.sagemath.org/ticket/9054#comment:1
<ul>
<li><strong>cc</strong>
<em>burcin</em> added
</li>
</ul>
TicketwasThu, 27 May 2010 01:27:22 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054-part2.patch</em>
</li>
</ul>
TicketwasThu, 27 May 2010 03:01:01 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054-part3.patch</em>
</li>
</ul>
TicketsalmanhbThu, 27 May 2010 05:38:49 GMT
https://trac.sagemath.org/ticket/9054#comment:2
https://trac.sagemath.org/ticket/9054#comment:2
<p>
There seems to be an issue with returning the base ring of a <a class="missing wiki">RationalFunctionField?</a>. Neither base() nor base_ring() return the correct ring:
</p>
<pre class="wiki">sage: K.<t> = FunctionField(QQ); K
Rational function field in t over Rational Field
sage: R1 = K.base(); R1
Rational function field in t over Rational Field
sage: R2 = K.base_ring(); R2
Rational function field in t over Rational Field
sage: R3.<s> = QQ[]; K3 = Frac(R3); K3
Fraction Field of Univariate Polynomial Ring in s over Rational Field
sage: R3
Univariate Polynomial Ring in s over Rational Field
sage: K3.base() == R3
True
</pre>
TicketwasThu, 27 May 2010 09:47:26 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054-part4.patch</em>
</li>
</ul>
TicketrobertwbThu, 27 May 2010 10:10:47 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054-part5.patch</em>
</li>
</ul>
TicketrobertwbThu, 27 May 2010 10:13:10 GMT
https://trac.sagemath.org/ticket/9054#comment:3
https://trac.sagemath.org/ticket/9054#comment:3
<p>
Looks like you forgot to add the file <code>function_field_order</code>, so I wasn't able to doctest on top of your latest patch (let alone debug it). See also <a class="closed ticket" href="https://trac.sagemath.org/ticket/9051" title="defect: Fast function field arithmetic (closed: fixed)">#9051</a> for added speed in the positive characteristic case.
</p>
TicketwasThu, 27 May 2010 20:43:31 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054-part6.patch</em>
</li>
</ul>
TicketwasThu, 27 May 2010 22:53:04 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054-part7.patch</em>
</li>
</ul>
<p>
polynomial factorization!
</p>
TicketjenFri, 28 May 2010 01:21:20 GMT
https://trac.sagemath.org/ticket/9054#comment:4
https://trac.sagemath.org/ticket/9054#comment:4
<p>
<a class="missing wiki">FunctionField?</a> constructor clips names
</p>
<pre class="wiki">sage: F = FunctionField(GF(7), 'bit')
sage: F.gen()
b
</pre>
TicketwasFri, 28 May 2010 06:29:45 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054-part8.patch</em>
</li>
</ul>
<p>
ideals and orders!
</p>
TicketwasFri, 28 May 2010 08:22:52 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054-part9.patch</em>
</li>
</ul>
<p>
inverses of fractional ideals
</p>
TicketwasFri, 28 May 2010 10:54:25 GMT
https://trac.sagemath.org/ticket/9054#comment:5
https://trac.sagemath.org/ticket/9054#comment:5
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9054#comment:2" title="Comment 2">salmanhb</a>:
</p>
<blockquote class="citation">
<p>
There seems to be an issue with returning the base ring of a <a class="missing wiki">RationalFunctionField?</a>. Neither base() nor base_ring() return the correct ring:
</p>
<pre class="wiki">sage: K.<t> = FunctionField(QQ); K
Rational function field in t over Rational Field
sage: R1 = K.base(); R1
Rational function field in t over Rational Field
sage: R2 = K.base_ring(); R2
Rational function field in t over Rational Field
sage: R3.<s> = QQ[]; K3 = Frac(R3); K3
Fraction Field of Univariate Polynomial Ring in s over Rational Field
sage: R3
Univariate Polynomial Ring in s over Rational Field
sage: K3.base() == R3
True
</pre></blockquote>
<p>
The above is correct. To get what you want, use the constant_field() method.
</p>
<pre class="wiki">sage: K.<t> = FunctionField(QQ);
sage: K.constant_field()
Rational Field
</pre>
TicketwasFri, 28 May 2010 10:55:53 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054-part10.patch</em>
</li>
</ul>
<p>
morphisms of function fields
</p>
TicketwasSat, 29 May 2010 03:13:02 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054-part11.patch</em>
</li>
</ul>
TicketrobertwbSun, 30 May 2010 09:54:48 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054-part12.patch</em>
</li>
</ul>
<p>
Various methods needed for <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/9095" title="enhancement: Elliptic curve over function field heights (needs_work)">#9095</a> (doctesets depend on <a class="closed ticket" href="https://trac.sagemath.org/ticket/9094" title="enhancement: is_square and sqrt for polynomials and fraction fields (closed: fixed)">#9094</a>)
</p>
TicketkhwilsonFri, 04 Jun 2010 22:17:13 GMT
https://trac.sagemath.org/ticket/9054#comment:6
https://trac.sagemath.org/ticket/9054#comment:6
<p>
Should be some automatic way to do the following:
</p>
<pre class="wiki">K.<T> = FunctionField(GF(17))
P = T-5
f = P^5
R = K._ring
R(f.element()).valuation(R(p.element()))
</pre>
TicketkhwilsonFri, 04 Jun 2010 22:17:58 GMTcc changed
https://trac.sagemath.org/ticket/9054#comment:7
https://trac.sagemath.org/ticket/9054#comment:7
<ul>
<li><strong>cc</strong>
<em>khwilson</em> added
</li>
</ul>
TicketwasTue, 06 Jul 2010 09:25:32 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054-part1-12.patch</em>
</li>
</ul>
<p>
flattened patch that incorporates all of patches 1-12 above into a single patch.
</p>
TicketwasTue, 06 Jul 2010 09:37:38 GMTowner changed
https://trac.sagemath.org/ticket/9054#comment:8
https://trac.sagemath.org/ticket/9054#comment:8
<ul>
<li><strong>owner</strong>
changed from <em>AlexGhitza</em> to <em>was</em>
</li>
</ul>
<p>
Here is a link to the result of doctesting sage-4.4.4 + patches 1-12:
</p>
<blockquote>
<p>
<a class="ext-link" href="http://sage.math.washington.edu/home/wstein/patches/9054-part1-12.doctest.txt"><span class="icon"></span>http://sage.math.washington.edu/home/wstein/patches/9054-part1-12.doctest.txt</a>
</p>
</blockquote>
<p>
The failed tests:
</p>
<pre class="wiki">The following tests failed:
sage -t devel/sage-main/sage/matrix/matrix2.pyx # 1 doctests failed
sage -t devel/sage-main/sage/plot/matrix_plot.py # 0 doctests failed
sage -t devel/sage-main/sage/modular/abvar/morphism.py # 1 doctests failed
sage -t devel/sage-main/sage/modular/abvar/finite_subgroup.py # 1 doctests failed
sage -t devel/sage-main/sage/tests/startup.py # 1 doctests failed
sage -t devel/sage-main/sage/modular/modform/hecke_operator_on_qexp.py # 1 doctests failed
sage -t devel/sage-main/sage/categories/function_fields.py # 5 doctests failed
sage -t devel/sage-main/sage/rings/function_field/function_field_element.pyx # 14 doctests failed
</pre>
TicketmderickxTue, 06 Jul 2010 11:03:27 GMTcc changed
https://trac.sagemath.org/ticket/9054#comment:9
https://trac.sagemath.org/ticket/9054#comment:9
<ul>
<li><strong>cc</strong>
<em>mderickx</em> added
</li>
</ul>
TicketmstrengTue, 06 Jul 2010 20:02:18 GMTcc changed
https://trac.sagemath.org/ticket/9054#comment:10
https://trac.sagemath.org/ticket/9054#comment:10
<ul>
<li><strong>cc</strong>
<em>mstreng</em> added
</li>
</ul>
TicketpbruinWed, 07 Jul 2010 09:34:59 GMTcc changed
https://trac.sagemath.org/ticket/9054#comment:11
https://trac.sagemath.org/ticket/9054#comment:11
<ul>
<li><strong>cc</strong>
<em>pbruin</em> added
</li>
</ul>
TicketwasWed, 07 Jul 2010 12:52:38 GMTdescription changed
https://trac.sagemath.org/ticket/9054#comment:12
https://trac.sagemath.org/ticket/9054#comment:12
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9054?action=diff&version=12">diff</a>)
</li>
</ul>
TicketwasWed, 07 Jul 2010 13:01:55 GMT
https://trac.sagemath.org/ticket/9054#comment:13
https://trac.sagemath.org/ticket/9054#comment:13
<p>
NOTE: <a class="closed ticket" href="https://trac.sagemath.org/ticket/9094" title="enhancement: is_square and sqrt for polynomials and fraction fields (closed: fixed)">#9094</a> implements sqrt for polynomials, which is relevant to trac_9054-doctest.patch
</p>
TicketmderickxWed, 07 Jul 2010 21:17:51 GMT
https://trac.sagemath.org/ticket/9054#comment:14
https://trac.sagemath.org/ticket/9054#comment:14
<p>
I guess the doctest patch isn't really usefull addition to sage (although making it was a usefull learning experience for Peter Bruin and me since it was our first patch). The patch fixes some bugs which are also fixed in other patches in trac. Some are indeed fixed by <a class="closed ticket" href="https://trac.sagemath.org/ticket/9094" title="enhancement: is_square and sqrt for polynomials and fraction fields (closed: fixed)">#9094</a> (although i think this can be done faster and more elegant) and another one related calculating the valuation in fraction fields is fixed by 9051-FpT_4.patch in <a class="closed ticket" href="https://trac.sagemath.org/ticket/9051" title="defect: Fast function field arithmetic (closed: fixed)">#9051</a>.
</p>
<p>
Since I'm quite new to developing and using trac and hg etc. I would like to know what is the best thing to do now. And especially how to deal with the relating patches wich also contain fixes for stuff happening here.
</p>
TicketwasThu, 08 Jul 2010 11:12:25 GMTdescription changed
https://trac.sagemath.org/ticket/9054#comment:15
https://trac.sagemath.org/ticket/9054#comment:15
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9054?action=diff&version=15">diff</a>)
</li>
</ul>
TicketmderickxSat, 17 Jul 2010 11:02:34 GMT
https://trac.sagemath.org/ticket/9054#comment:16
https://trac.sagemath.org/ticket/9054#comment:16
<p>
Added an attachment that fixes all but three doctest failures. The remaining failures are:
</p>
<pre class="wiki">
sage -t "devel/sage-mderickx/sage/modular/abvar/morphism.py" # 1 failure
sage -t "devel/sage-mderickx/sage/modular/abvar/finite_subgroup.py" # 1 failure
sage -t "devel/sage-main/sage/modular/modform/hecke_operator_on_qexp.py" # 1 failure
}}}They are all related since their error messages all end in:{{{ File "/Applications/sage/local/lib/python/site-packages/sage/modules/free_module.py", line 4700, in _echelonized_basis d = self._denominator(basis) File "/Applications/sage/local/lib/python/site-packages/sage/modules/free_module.py", line 4810, in _denominator d = d.lcm(x.denominator()) !AttributeError: 'int' object has no attribute 'lcm'}}}It would be nice if someone who has a better understanding of sage to fix this final bug, since then we would have no doctests failing anymore for this patch.
</pre>
TicketmderickxSat, 17 Jul 2010 11:05:00 GMT
https://trac.sagemath.org/ticket/9054#comment:17
https://trac.sagemath.org/ticket/9054#comment:17
<p>
Oeps, wrong fromatting. Now a bit more readable:
</p>
<pre class="wiki">sage -t "devel/sage-mderickx/sage/modular/abvar/morphism.py" # 1 failure
sage -t "devel/sage-mderickx/sage/modular/abvar/finite_subgroup.py" # 1 failure
sage -t "devel/sage-main/sage/modular/modform/hecke_operator_on_qexp.py" # 1 failure
</pre><p>
They are all related since their error messages all end in:
</p>
<pre class="wiki">
File "/Applications/sage/local/lib/python/site-packages/sage/modules/free_module.py", line 4700, in _echelonized_basis
d = self._denominator(basis)
File "/Applications/sage/local/lib/python/site-packages/sage/modules/free_module.py", line 4810, in _denominator
d = d.lcm(x.denominator())
AttributeError: 'int' object has no attribute 'lcm'
</pre>
TicketnovoseltSat, 17 Jul 2010 16:15:10 GMTcc changed
https://trac.sagemath.org/ticket/9054#comment:18
https://trac.sagemath.org/ticket/9054#comment:18
<ul>
<li><strong>cc</strong>
<em>novoselt</em> added
</li>
</ul>
TicketminzSat, 17 Jul 2010 19:35:43 GMTcc changed
https://trac.sagemath.org/ticket/9054#comment:19
https://trac.sagemath.org/ticket/9054#comment:19
<ul>
<li><strong>cc</strong>
<em>minz</em> added
</li>
</ul>
TicketmderickxFri, 30 Jul 2010 18:36:51 GMT
https://trac.sagemath.org/ticket/9054#comment:20
https://trac.sagemath.org/ticket/9054#comment:20
<p>
Has there been any work on this since sage days > 23? Even if the work is only partially finnished it would be good to know to avoid double work.
</p>
TicketmderickxSat, 31 Jul 2010 16:08:17 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054-doctest.patch</em>
</li>
</ul>
<p>
Aplies to sage 4.4.4 after 1-12 patch and it also needs the <a class="closed ticket" href="https://trac.sagemath.org/ticket/9054" title="enhancement: create a class for basic function_field arithmetic for Sage (closed: fixed)">#9054</a> patch trac_9094-sqrt-mderickx.patch
</p>
TicketwasWed, 25 Aug 2010 05:34:08 GMT
https://trac.sagemath.org/ticket/9054#comment:21
https://trac.sagemath.org/ticket/9054#comment:21
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9054#comment:20" title="Comment 20">mderickx</a>:
</p>
<blockquote class="citation">
<p>
Has there been any work on this since sage days > 23? Even if the work is only partially
finished it would be good to know to avoid double work.
</p>
</blockquote>
<p>
There has been no further work. When I do work further on this, I will post a patch. I always post patches of everything I do as I go, as soon as I'm done with a session of work.
</p>
TicketwasTue, 26 Oct 2010 15:49:42 GMT
https://trac.sagemath.org/ticket/9054#comment:22
https://trac.sagemath.org/ticket/9054#comment:22
<p>
I am moving this entirely out of Sage and into the psage library. See
</p>
<blockquote>
<p>
<a class="ext-link" href="http://code.google.com/p/purplesage/issues/detail?id=3"><span class="icon"></span>http://code.google.com/p/purplesage/issues/detail?id=3</a>
</p>
</blockquote>
TicketwasTue, 26 Oct 2010 22:47:35 GMTstatus changed; resolution set
https://trac.sagemath.org/ticket/9054#comment:23
https://trac.sagemath.org/ticket/9054#comment:23
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>wontfix</em>
</li>
</ul>
<p>
I'm closing this since I'm no longer interested in getting it included in the main sage distribution. It is now in psage as mentioned above.
</p>
TicketmvnguWed, 27 Oct 2010 08:20:55 GMTmilestone changed
https://trac.sagemath.org/ticket/9054#comment:24
https://trac.sagemath.org/ticket/9054#comment:24
<ul>
<li><strong>milestone</strong>
changed from <em>sage-4.6</em> to <em>sage-duplicate/invalid/wontfix</em>
</li>
</ul>
TicketrobertwbWed, 27 Oct 2010 16:05:50 GMTstatus, milestone changed; resolution deleted
https://trac.sagemath.org/ticket/9054#comment:25
https://trac.sagemath.org/ticket/9054#comment:25
<ul>
<li><strong>status</strong>
changed from <em>closed</em> to <em>new</em>
</li>
<li><strong>resolution</strong>
<em>wontfix</em> deleted
</li>
<li><strong>milestone</strong>
changed from <em>sage-duplicate/invalid/wontfix</em> to <em>sage-wishlist</em>
</li>
</ul>
<p>
I think eventually this should be in the main sage distribution, even if no one's actively working on it right now.
</p>
TicketminzThu, 31 Mar 2011 15:14:29 GMT
https://trac.sagemath.org/ticket/9054#comment:26
https://trac.sagemath.org/ticket/9054#comment:26
<p>
There should be no doctest failures left. Comments, remarks, and reviews are welcome. :)
</p>
<p>
Apply trac_9054-all-parts.patch<br />
Depends on <a class="closed ticket" href="https://trac.sagemath.org/ticket/9094" title="enhancement: is_square and sqrt for polynomials and fraction fields (closed: fixed)">#9094</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/11034" title="defect: bug in the way vectors are created for submodules of free modules (closed: duplicate)">#11034</a>
</p>
TicketminzFri, 01 Apr 2011 08:40:09 GMTdescription changed; author set
https://trac.sagemath.org/ticket/9054#comment:27
https://trac.sagemath.org/ticket/9054#comment:27
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9054?action=diff&version=27">diff</a>)
</li>
<li><strong>author</strong>
set to <em>William Stein, Robert Bradshaw, Maarten Derickx, Moritz Minzlaff</em>
</li>
</ul>
TicketsaraedumMon, 06 Jun 2011 15:23:37 GMTcc changed
https://trac.sagemath.org/ticket/9054#comment:28
https://trac.sagemath.org/ticket/9054#comment:28
<ul>
<li><strong>cc</strong>
<em>saraedum</em> added
</li>
</ul>
TicketsaraedumWed, 08 Jun 2011 19:05:58 GMT
https://trac.sagemath.org/ticket/9054#comment:29
https://trac.sagemath.org/ticket/9054#comment:29
<p>
The doctests of <code>function_field.py</code> contain the following lines:
</p>
<pre class="wiki">sage: R.<x> = FunctionField(QQ); S.<y> = R[]
sage: R2.<t> = FunctionField(QQ); S2.<w> = R2[]
sage: L2.<w> = R.extension((4*w)^2 - (t+1)^3 - 1)
</pre><p>
I think it is confusing that it does not make a difference whether you write R.extension or R2.extension in this example. I'm new to sage so maybe I'm misunderstanding something here.
</p>
TicketsaraedumWed, 08 Jun 2011 19:56:44 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054_polynomial_base_field.patch</em>
</li>
</ul>
<p>
polynomial used for a field extension must be defined over the base field
</p>
TicketsaraedumTue, 28 Jun 2011 17:22:53 GMT
https://trac.sagemath.org/ticket/9054#comment:30
https://trac.sagemath.org/ticket/9054#comment:30
<p>
There are some problems with the zero of a function field:
</p>
<pre class="wiki">sage: K.<x> = FunctionField(QQ); R.<y> = K[]; L.<y> = K.extension(y^2+x);
sage: coerce(L,L.polynomial())==0
False
sage: y/0
0
</pre>
TicketsaraedumTue, 28 Jun 2011 17:23:47 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054_zero.patch</em>
</li>
</ul>
<p>
fixes the problems regarding zero.
</p>
TicketsaraedumFri, 22 Jul 2011 16:26:22 GMT
https://trac.sagemath.org/ticket/9054#comment:31
https://trac.sagemath.org/ticket/9054#comment:31
<p>
Entering the following at the sage prompt produces a <code>TypeError: Unable to coerce -u^2 (...) to Rational</code>.
</p>
<pre class="wiki">K.<x> = FunctionField(QQ); R.<y> = K[]
L.<y> = K.extension(y^2 - x)
M.<u> = FunctionField(QQ); R.<v> = M[]
N.<v> = M.extension(v-u^2)
L.hom([u,v])
</pre><p>
This is due to the fact that <code>hom()</code> determines the codomain by looking only at the first element of <code>[u,v]</code>.
</p>
TicketsaraedumFri, 22 Jul 2011 16:27:07 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054_codomain.patch</em>
</li>
</ul>
<p>
set the correct codomain for function fields
</p>
TicketsaraedumFri, 22 Jul 2011 16:49:18 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054_doctest-2.patch</em>
</li>
</ul>
<p>
fixes hash doctests for 32bit and a random doctest
</p>
TicketsaraedumFri, 22 Jul 2011 17:31:08 GMT
https://trac.sagemath.org/ticket/9054#comment:32
https://trac.sagemath.org/ticket/9054#comment:32
<p>
Is there a reason why a FunctionFieldMorphism is a Map and not a RingHomomorphism?
</p>
TicketsaraedumThu, 25 Aug 2011 05:04:33 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054_function_fields_sd32.patch</em>
</li>
</ul>
<p>
Minimal support for functions field. Does not include all of the above patches.
</p>
TicketmderickxFri, 26 Aug 2011 22:27:24 GMT
https://trac.sagemath.org/ticket/9054#comment:33
https://trac.sagemath.org/ticket/9054#comment:33
<p>
I'm now busy with very troughly checking the entire patch wich at least with some changed free module stuff passes all doctests. There will be a big doctest patch comming up which includes tests I've thought up to also test some more none trivial examples.
</p>
<p>
There is are at least two big issues which I run in to today. They all occured in the same terminal session.
</p>
<pre class="wiki">sage: K.<x> = FunctionField(QQ)
sage: R.<y> = K[]
sage: L.<w> = K.extension(y^5 - (x^3 + 2*x*y + 1/x));
sage: w.is_integral()
False
sage: L.order(w) #should raise a value error since orders can only be generated by integral elements
Order in Function field in w defined by y^5 - 2*x*y + (-x^4 - 1)/x
sage: L.order(w).gens()
---------------------------------------------------------------------------
RuntimeError Traceback (most recent call last)
/Users/maarten/Downloads/sage-4.7.2.alpha2/devel/sage-main/<ipython console> in <module>()
/Users/maarten/Downloads/sage-4.7.2.alpha2/local/lib/python2.6/site-packages/sage/structure/parent_gens.so in sage.structure.parent_gens.ParentWithGens.gens (sage/structure/parent_gens.c:2741)()
/Users/maarten/Downloads/sage-4.7.2.alpha2/local/lib/python2.6/site-packages/sage/structure/parent_gens.so in sage.structure.parent_gens.ParentWithGens.ngens (sage/structure/parent_gens.c:2548)()
/Users/maarten/Downloads/sage-4.7.2.alpha2/local/lib/python2.6/site-packages/sage/structure/parent_gens.so in sage.structure.parent_gens.check_old_coerce (sage/structure/parent_gens.c:1228)()
RuntimeError: Order in Function field in w defined by y^5 - 2*x*y + (-x^4 - 1)/x still using old coercion framework
</pre>
TicketmderickxSun, 28 Aug 2011 06:11:58 GMT
https://trac.sagemath.org/ticket/9054#comment:34
https://trac.sagemath.org/ticket/9054#comment:34
<p>
the review patch is not entirely ready, but julian wanted to have a look so I uploaded it
</p>
TicketSimonKingSun, 28 Aug 2011 06:58:28 GMT
https://trac.sagemath.org/ticket/9054#comment:35
https://trac.sagemath.org/ticket/9054#comment:35
<p>
At <a class="ext-link" href="http://groups.google.com/group/sage-devel/browse_thread/thread/c62fa3dae0a2ca82"><span class="icon"></span>sage-devel</a>, Maarten mentioned that pickling does not seem to work for the code posted here, which seems to be due to some attributes typically involved in coercion.
</p>
<p>
Looking at <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9054/trac_9054-all-parts.patch" title="Attachment 'trac_9054-all-parts.patch' in Ticket #9054">trac_9054-all-parts.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9054/trac_9054-all-parts.patch" title="Download"></a>, I see that the base class for function fields is derived from <code>sage.rings.ring.Field</code>, but <code>Field.__init__</code> is not called.
</p>
<p>
The rings in vanilla Sage do not pay enough attention to coercion and categories. <a class="closed ticket" href="https://trac.sagemath.org/ticket/9944" title="defect: categories for polynomial rings (closed: fixed)">#9944</a> and (in particular) <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a> aim at improving the situation. In particular, with <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a> it should now possible to avoid any direct call to <code>ParentWithGens.__init__</code>; calling <code>Field.__init__</code> should just work (tm). Can you try?
</p>
TicketSimonKingSun, 28 Aug 2011 07:03:44 GMT
https://trac.sagemath.org/ticket/9054#comment:36
https://trac.sagemath.org/ticket/9054#comment:36
<p>
PS: After <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9054/trac_9054-all-parts.patch" title="Attachment 'trac_9054-all-parts.patch' in Ticket #9054">trac_9054-all-parts.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9054/trac_9054-all-parts.patch" title="Download"></a> was created, several other patches were posted. Can you please clearly state (in the ticket description and, for the patchbot, also in a comment) which patches are supposed to be applied? It is difficult to work on the pickling problem (or reviewing) if it is not clear what code exactly we are talking about.
</p>
TicketSimonKingSun, 28 Aug 2011 08:02:43 GMT
https://trac.sagemath.org/ticket/9054#comment:37
https://trac.sagemath.org/ticket/9054#comment:37
<p>
Here are some comments on <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9054/trac_9054-all-parts.patch" title="Attachment 'trac_9054-all-parts.patch' in Ticket #9054">trac_9054-all-parts.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9054/trac_9054-all-parts.patch" title="Download"></a>:
</p>
<ul><li>Please remove the <code>__contains__</code> method from the category <code>FunctionFields</code>. Containment in categories should rely on the default implementation, unless there is a compelling reason to do otherwise.
</li></ul><blockquote>
<p>
Even worse, your containment test is ultimately based on testing class inheritance (namely in the function <code>is_FunctionField</code>). That totally undermines the category framework. It must be possible for an object to be a function field even without inheriting from <code>sage.rings.function_field.function_field.FunctionField</code>.
</p>
</blockquote>
<blockquote>
<p>
The default implementation of <code>F in FunctionFields()</code> relies on the category of F: The containment test returns True if and only if <code>F.category()</code> is a sub-category of <code>FunctionFields()</code>. That should be much better, from a mathematical point of view, than testing class inheritance!
</p>
</blockquote>
<ul><li>You should add a test of the form <code>TestSuite(F).run()</code>, where F is a function field. The test suite is formed by some generic tests defined in the category framework and includes many sanity tests (such as pickling for the field and its elements, associativity, commtativity, ...). If you can think of specific tests for function fields, then you should add methods named <code>_test_...</code> as parent or element methods of <code>sage.categories.function_fields.FunctionFields</code>. Such methods will be automatically called when running <code>TestSuite(F).run()</code>.
</li></ul><ul><li>You should also add a test of the form <code>loads(dumps(F)) is F</code>, in order to test uniqueness of parent structures; if I recall correctly, the test suite from the category would only test <code>loads(dumps(F))==F</code>.
</li></ul><ul><li>It should not be needed to have a function <code>is_FunctionField</code> (that just tests class inheritance) - <code>F in FunctionFields()</code> is a better test, IMHO. If you do want to preserve <code>is_FunctionField</code> then please do not simply put it in the global name space. At least, it should be deprecated, similar to <code>is_Ring</code> being deprecated. There is a function decorator to do so.
</li></ul><ul><li>In the doc test for the <code>_element_constructor_</code> method, you explicitly call the method. I think it should better be an indirect test (after all, the documentation is supposed to show how the user is supposed to work with stuff). Hence, not <code>L._element_constructor_(L.polynomial_ring().gen())</code> but <code>L(L.polynomial_ring().gen()) #indirect doctest</code>.
</li></ul><ul><li>I already mentioned, since <code>FunctionField</code> is derived from <code>sage.rings.ring.Field</code>, that <code>Field.__init__(...)</code> should be called. It could be that this only works when <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a> is used. Just calling <code>ParentWithGens.__init__</code> may be insufficient.
</li></ul><ul><li>There are several methods, such as polynomial_ring or vector_space, that use a hand-made cache. Please use the @cached_method decorator instead! That has several reasons.
<ol><li>It is more easy. You don't need to manually update attributes.
</li><li>With <a class="closed ticket" href="https://trac.sagemath.org/ticket/11115" title="enhancement: Rewrite cached_method in Cython (closed: fixed)">#11115</a>, the @cached_method decorator is rewritten in Cython and provides a faster cache than anything you could possibly create with Python.
</li></ol></li></ul><ul><li>Is there a reason why you have a method <code>base_field</code> that simply returns the function field itself? From the behaviour of the <code>base_ring</code> method of polynomial rings, I would rather expect that <code>FunctionField(QQ,['t']).base_field()</code> returns the rational field.
</li></ul>
TicketSimonKingSun, 28 Aug 2011 16:17:56 GMT
https://trac.sagemath.org/ticket/9054#comment:38
https://trac.sagemath.org/ticket/9054#comment:38
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9054#comment:37" title="Comment 37">SimonKing</a>:
</p>
<blockquote class="citation">
<ul><li>I already mentioned,
</li></ul></blockquote>
<p>
... namely on <a class="ext-link" href="http://groups.google.com/group/sage-devel/browse_thread/thread/c62fa3dae0a2ca82"><span class="icon"></span>sage-devel</a>,
</p>
<blockquote class="citation">
<p>
that <code>Field.__init__(...)</code> should be called. It could be that this only works when <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a> is used. Just calling <code>ParentWithGens.__init__</code> may be insufficient.
</p>
</blockquote>
TicketwasSun, 28 Aug 2011 16:35:09 GMT
https://trac.sagemath.org/ticket/9054#comment:39
https://trac.sagemath.org/ticket/9054#comment:39
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9054#comment:37" title="Comment 37">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Here are some comments on <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9054/trac_9054-all-parts.patch" title="Attachment 'trac_9054-all-parts.patch' in Ticket #9054">trac_9054-all-parts.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9054/trac_9054-all-parts.patch" title="Download"></a>:
</p>
<ul><li>Please remove the <code>__contains__</code> method from the category <code>FunctionFields</code>. Containment in categories should rely on the default implementation, unless there is a compelling reason to do otherwise.
</li></ul><blockquote>
<p>
Even worse, your containment test is ultimately based on testing class inheritance (namely in the function <code>is_FunctionField</code>). That totally undermines the category framework. It must be possible for an object to be a function field even without inheriting from <code>sage.rings.function_field.function_field.FunctionField</code>.
</p>
</blockquote>
<blockquote>
<p>
The default implementation of <code>F in FunctionFields()</code> relies on the category of F: The containment test returns True if and only if <code>F.category()</code> is a sub-category of <code>FunctionFields()</code>. That should be much better, from a mathematical point of view, than testing class inheritance!
</p>
</blockquote>
</blockquote>
<p>
Technically this is true. But this category framework instead of inheritance -- really two very different approaches to design -- leads directly to slow code in some cases in practice, which is *really* annoying, IMHO. For example, see <a class="closed ticket" href="https://trac.sagemath.org/ticket/11657" title="defect: the vector(...) function is extremely slow (closed: fixed)">#11657</a>, where one of the root causes of slowness was code in is_Ring that was added to support this category approach, and which slowed everything down. Fortunately for me I have psage where I can write streamlined code without having to be weighed down, and for generic Sage working well and being extensible is more important, so of course I agree with you in this case.
</p>
<blockquote class="citation">
<ul><li>You should add a test of the form <code>TestSuite(F).run()</code>, where F is a function field. The test suite is formed by some generic tests defined in the category framework and includes many sanity tests (such as pickling for the field and its elements, associativity, commtativity, ...). If you can think of specific tests for function fields, then you should add methods named <code>_test_...</code> as parent or element methods of <code>sage.categories.function_fields.FunctionFields</code>. Such methods will be automatically called when running <code>TestSuite(F).run()</code>.
</li></ul><ul><li>You should also add a test of the form <code>loads(dumps(F)) is F</code>, in order to test uniqueness of parent structures; if I recall correctly, the test suite from the category would only test <code>loads(dumps(F))==F</code>.
</li></ul></blockquote>
<p>
This is also testing that pickling works at all. This code is used by the pickle jar to create pickles for testing later.
</p>
<blockquote class="citation">
<ul><li>It should not be needed to have a function <code>is_FunctionField</code> (that just tests class inheritance) - <code>F in FunctionFields()</code> is a better test, IMHO. If you do want to preserve <code>is_FunctionField</code> then please do not simply put it in the global name space. At least, it should be deprecated, similar to <code>is_Ring</code> being deprecated. There is a function decorator to do so.
</li></ul></blockquote>
<p>
is_Ring is only deprecated when used from the top level (i.e., the Sage prompt). However, there is still a is_Ring function, which can be used in library code, and is not deprecated for this purpose. And the is_Ring function does test for category stuff.
</p>
<blockquote class="citation">
<ul><li>In the doc test for the <code>_element_constructor_</code> method, you explicitly call the method. I think i
</li></ul></blockquote>
<p>
t should better be an indirect test (after all, the documentation is supposed to show how the user is supposed to work with stuff). Hence, not <code>L._element_constructor_(L.polynomial_ring().gen())</code> but <code>L(L.polynomial_ring().gen()) #indirect doctest</code>.
</p>
<blockquote class="citation">
</blockquote>
<p>
I disagree. I view "#indirect test" for situations where you can't think of a clean way of directly calling the function. If there is such a way, use it! That way, at least you know for sure it is really being tested. Suggesting to get rid of that makes no sense to me. What if <code>L(L.polynomial_ring().gen())</code> doesn't call <code>_element_constructor_</code> at all? Also, one can also just have two tests -- one that is indirect and one that isn't.
</p>
<blockquote class="citation">
<ul><li>I already mentioned, since <code>FunctionField</code> is derived from <code>sage.rings.ring.Field</code>, that <code>Field.__init__(...)</code> should be called. It could be that this only works when <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a> is used. Just calling <code>ParentWithGens.__init__</code> may be insufficient.
</li></ul><ul><li>There are several methods, such as polynomial_ring or vector_space, that use a hand-made cache. Please use the @cached_method decorator instead! That has several reasons.
<ol><li>It is more easy. You don't need to manually update attributes.
</li><li>With <a class="closed ticket" href="https://trac.sagemath.org/ticket/11115" title="enhancement: Rewrite cached_method in Cython (closed: fixed)">#11115</a>, the @cached_method decorator is rewritten in Cython and provides a faster cache than anything you could possibly create with Python.
</li></ol></li></ul></blockquote>
<p>
+1. Note that when the very first version of the function field code was written (by me) @cached_method was disturbingly slow. I really, really appreciate the fast Cython rewrite.
</p>
<blockquote class="citation">
<ul><li>Is there a reason why you have a method <code>base_field</code> that simply returns the function field itself? From the behaviour of the <code>base_ring</code> method of polynomial rings, I would rather expect that <code>FunctionField(QQ,['t']).base_field()</code> returns the rational field.
</li></ul></blockquote>
<p>
No. The base field of a function field is a rational function field in 1 variable. The base field of that rational function field is then a field such as QQ. Most function fields aren't rational, e.g., they are finite extensions K/QQ(t), or even relative extensions L/K. In the first case, the base field is QQ(t) and in the second it is K.
If Simon was confused by this, it should be documented better.
</p>
<blockquote class="citation">
</blockquote>
TicketSimonKingSun, 28 Aug 2011 17:42:58 GMT
https://trac.sagemath.org/ticket/9054#comment:40
https://trac.sagemath.org/ticket/9054#comment:40
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9054#comment:39" title="Comment 39">was</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9054#comment:37" title="Comment 37">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Here are some comments on <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9054/trac_9054-all-parts.patch" title="Attachment 'trac_9054-all-parts.patch' in Ticket #9054">trac_9054-all-parts.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9054/trac_9054-all-parts.patch" title="Download"></a>:
</p>
<ul><li>Please remove the <code>__contains__</code> method from the category <code>FunctionFields</code>. Containment in categories should rely on the default implementation, unless there is a compelling reason to do otherwise.
</li></ul></blockquote>
<p>
...
</p>
<p>
Technically this is true. But this category framework instead of inheritance -- really two very different approaches to design -- leads directly to slow code in some cases in practice, which is *really* annoying, IMHO.
</p>
</blockquote>
<p>
A while ago, I had worked on a ticket <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/10667" title="enhancement: Morphisms and Objects of Categories (needs_work)">#10667</a> about category containment. One purpose was to get a speedup. The trick was (again) to use Cython. For some reason, the work on that ticket has stalled. Perhaps it would be worth while to resume it.
</p>
<p>
Generally, I think it is better to improve the category framework, rather than to work around it.
</p>
<blockquote class="citation">
<blockquote>
<p>
For example, see <a class="closed ticket" href="https://trac.sagemath.org/ticket/11657" title="defect: the vector(...) function is extremely slow (closed: fixed)">#11657</a>, where one of the root causes of slowness was code in is_Ring that was added to support this category approach, and which slowed everything down.
</p>
</blockquote>
</blockquote>
<p>
Then why is the existing <code>is_Ring</code> not rewritten along the lines of what you do in <a class="closed ticket" href="https://trac.sagemath.org/ticket/11657" title="defect: the vector(...) function is extremely slow (closed: fixed)">#11657</a>?
</p>
<blockquote class="citation">
<p>
is_Ring is only deprecated when used from the top level (i.e., the Sage prompt).
</p>
</blockquote>
<p>
Yes, this is what I meant. I did not mean "deprecated" in the sense of "will soon be removed", but in the sense of "please don't try this at home".
</p>
<blockquote class="citation">
<blockquote>
<p>
And the is_Ring function does test for category stuff.
</p>
</blockquote>
</blockquote>
<p>
Actually I have not been aware that category stuff is tested in <code>is_Ring</code>. I was thinking about various other <code>is_...</code> methods that really do nothing more than isinstance.
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>Is there a reason why you have a method <code>base_field</code> that simply returns the function field itself? From the behaviour of the <code>base_ring</code> method of polynomial rings, I would rather expect that <code>FunctionField(QQ,['t']).base_field()</code> returns the rational field.
</li></ul></blockquote>
<p>
No. The base field of a function field is a rational function field in 1 variable.
</p>
</blockquote>
<p>
Ouch, so I was mistaken.
</p>
<blockquote class="citation">
<p>
The base field of that rational function field is then a field such as QQ. Most function fields aren't rational, e.g., they are finite extensions K/QQ(t), or even relative extensions L/K. In the first case, the base field is QQ(t) and in the second it is K.
If Simon was confused by this, it should be documented better.
</p>
</blockquote>
<p>
Not needed. What I stated was based on reading the patch "diagonally". I only noticed one of the two base_field methods.
</p>
TicketwasSun, 28 Aug 2011 17:56:42 GMT
https://trac.sagemath.org/ticket/9054#comment:41
https://trac.sagemath.org/ticket/9054#comment:41
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9054#comment:40" title="Comment 40">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
A while ago, I had worked on a ticket <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/10667" title="enhancement: Morphisms and Objects of Categories (needs_work)">#10667</a> about category containment. One purpose was to get a speedup. The trick was (again) to use Cython. For some reason, the work on that ticket has stalled. Perhaps it would be worth while to resume it.
</p>
</blockquote>
<p>
+1
</p>
<blockquote class="citation">
<p>
Generally, I think it is better to improve the category framework, rather than to work around it.
</p>
<blockquote class="citation">
<blockquote>
<p>
For example, see <a class="closed ticket" href="https://trac.sagemath.org/ticket/11657" title="defect: the vector(...) function is extremely slow (closed: fixed)">#11657</a>, where one of the root causes of slowness was code in is_Ring that was added to support this category approach, and which slowed everything down.
</p>
</blockquote>
</blockquote>
<p>
Then why is the existing <code>is_Ring</code> not rewritten along the lines of what you do in <a class="closed ticket" href="https://trac.sagemath.org/ticket/11657" title="defect: the vector(...) function is extremely slow (closed: fixed)">#11657</a>?
</p>
</blockquote>
<p>
What I did there slows down <code>is_Ring</code> testing if the object in question does not derive from Ring.
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
is_Ring is only deprecated when used from the top level (i.e., the Sage prompt).
</p>
</blockquote>
<p>
Yes, this is what I meant. I did not mean "deprecated" in the sense of "will soon be removed", but in the sense of "please don't try this at home".
</p>
</blockquote>
<p>
If you are developing on the Sage library, I think it is OK to use.
</p>
<blockquote class="citation">
<blockquote class="citation">
<blockquote>
<p>
And the is_Ring function does test for category stuff.
</p>
</blockquote>
</blockquote>
<p>
Actually I have not been aware that category stuff is tested in <code>is_Ring</code>. I was thinking about various other <code>is_...</code> methods that really do nothing more than isinstance.
</p>
</blockquote>
<p>
Yes, take a look at the code. I too was surprised by this!
</p>
<blockquote>
<p>
-- William
</p>
</blockquote>
TicketmderickxSun, 28 Aug 2011 19:09:53 GMTdescription changed; dependencies set
https://trac.sagemath.org/ticket/9054#comment:42
https://trac.sagemath.org/ticket/9054#comment:42
<ul>
<li><strong>dependencies</strong>
set to <em>#9094, #11034</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/9054?action=diff&version=42">diff</a>)
</li>
</ul>
<p>
I changed the description so that it's clear which code to look at. I will read the rest of all the remarks when I'm back from lunch.
</p>
TicketmderickxMon, 29 Aug 2011 02:44:11 GMT
https://trac.sagemath.org/ticket/9054#comment:43
https://trac.sagemath.org/ticket/9054#comment:43
<p>
Dear Simon,
</p>
<p>
Thanks for the help and suggestions. But sadly it did not help (altough I find <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a> a very cool ticket it's good to make a lot of rings finally more consistent with the current model of doing things with the category framework).
</p>
<p>
After some fiddeling around I managed to reduce the error to something in FunctionFieldElement_rational initialization code (hence probably not something with the categorie an coercion framework).
</p>
<pre class="wiki">sage: K = QQ['x'].fraction_field(); x = K.gen(0)
sage: sage.rings.function_field.function_field_element.FunctionFieldElement_rational(K, x)
x
sage: l=sage.rings.function_field.function_field_element.FunctionFieldElement_rational(K, x)
sage: dumps(l)
PicklingError Traceback (most recent call last)
...
PicklingError: Can't pickle <type 'dictproxy'>: attribute lookup __builtin__.dictproxy failed
sage: l.__getstate__()
(Fraction Field of Univariate Polynomial Ring in x over Rational Field, <dictproxy object at 0x10ddf9948>)
</pre>
TicketmderickxMon, 29 Aug 2011 06:51:00 GMT
https://trac.sagemath.org/ticket/9054#comment:44
https://trac.sagemath.org/ticket/9054#comment:44
<p>
It took me a while to find out how to solve the problems with pickling but I finally managed to do so. It was because of cython objects not being pickleable automatically so you have to write your own pickling methods. A more experienced programmer might have found this out way faster then me, but I had a lot of trouble (basically spent this entire afternoon reading about how pickling protocol works so I could fix it. I will now look into the issues you described and get a definite patch up.
</p>
TicketSimonKingMon, 29 Aug 2011 09:57:11 GMT
https://trac.sagemath.org/ticket/9054#comment:45
https://trac.sagemath.org/ticket/9054#comment:45
<p>
Just for your information: I resumed work on <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/10667" title="enhancement: Morphisms and Objects of Categories (needs_work)">#10667</a>.
</p>
<p>
Testing whether QQ is a ring works faster with the methods from <a class="closed ticket" href="https://trac.sagemath.org/ticket/11115" title="enhancement: Rewrite cached_method in Cython (closed: fixed)">#11115</a> and <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/10667" title="enhancement: Morphisms and Objects of Categories (needs_work)">#10667</a> than with using the current <code>is_Ring</code>:
</p>
<pre class="wiki">sage: C = CommutativeRings().objects()
sage: QQ in C
True
sage: %timeit QQ in C
625 loops, best of 3: 3.88 µs per loop
</pre><p>
versus
</p>
<pre class="wiki">sage: from sage.rings.ring import is_Ring
sage: %timeit is_Ring(QQ)
625 loops, best of 3: 5.06 µs per loop
</pre><p>
Of course, just testing the class is a lot faster:
</p>
<pre class="wiki">sage: from sage.rings.ring import Ring
sage: %timeit isinstance(QQ,Ring)
625 loops, best of 3: 666 ns per loop
</pre>
TicketSimonKingMon, 29 Aug 2011 11:09:43 GMT
https://trac.sagemath.org/ticket/9054#comment:46
https://trac.sagemath.org/ticket/9054#comment:46
<p>
I really think that <code>is_Ring</code> should be <em>globally</em> improved. For example, it already helps to define
</p>
<pre class="wiki">def is_Ring(x):
"""
Return True if x is a ring.
EXAMPLES::
sage: from sage.rings.ring import is_Ring
sage: is_Ring(ZZ)
True
"""
if isinstance(x, Ring):
return True
from sage.categories.rings import Rings
return x in Rings()
</pre><p>
hence, only do the import when needed.
</p>
<p>
The timings become
</p>
<pre class="wiki">sage: from sage.rings.ring import is_Ring
sage: P.<x,y,z> = QQ[]
sage: is_Ring(P)
True
sage: %timeit is_Ring(P)
625 loops, best of 3: 243 ns per loop
sage: MS = MatrixSpace(QQ,2)
sage: is_Ring(MS)
True
sage: %timeit is_Ring(MS)
625 loops, best of 3: 21.5 µs per loop
</pre><p>
versus
</p>
<pre class="wiki">sage: from sage.rings.ring import is_Ring
sage: sage: P.<x,y,z> = QQ[]
sage: is_Ring(P)
True
sage: %timeit is_Ring(P)
625 loops, best of 3: 4.93 µs per loop
sage: MS = MatrixSpace(QQ,2)
sage: sage: is_Ring(MS)
True
sage: %timeit is_Ring(MS)
625 loops, best of 3: 26.4 µs per loop
</pre><p>
But I think I'll move it to <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/10667" title="enhancement: Morphisms and Objects of Categories (needs_work)">#10667</a>.
</p>
TicketSimonKingTue, 30 Aug 2011 10:09:19 GMT
https://trac.sagemath.org/ticket/9054#comment:47
https://trac.sagemath.org/ticket/9054#comment:47
<p>
Time for a little advertisement: I obtain a much improved performance with <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/10667" title="enhancement: Morphisms and Objects of Categories (needs_work)">#10667</a> (introducing the class of objects and morphisms of a category, written in Cython). Perhaps it is useful for you?
</p>
<p>
<strong><span class="underline">Testing commutative rings</span></strong>
</p>
<p>
The function <code>is_CommutativeRing</code> does nothing but testing the class. But it is a Python function. Let us compare its speed with the speed of a Cython container, testing category containment.
</p>
<p>
<code>is_CommutativeRing</code>:
</p>
<pre class="wiki">sage: from sage.rings.commutative_ring import is_CommutativeRing
sage: is_CommutativeRing??
...
Source:
def is_CommutativeRing(R):
return isinstance(R, CommutativeRing)
sage: is_CommutativeRing(QQ)
True
sage: s = SymmetricGroup(4)
sage: is_CommutativeRing(s)
False
sage: %timeit is_CommutativeRing(QQ)
625 loops, best of 3: 1.09 µs per loop
sage: %timeit is_CommutativeRing(s)
625 loops, best of 3: 3.51 µs per loop
</pre><p>
Cython container:
</p>
<pre class="wiki">sage: O = CommutativeRings().objects()
sage: QQ in O
True
sage: s in O
False
sage: %timeit QQ in O
625 loops, best of 3: 1.5 µs per loop
sage: %timeit s in O
625 loops, best of 3: 1.46 µs per loop
</pre><p>
Hence, when applied to a symmetric group, the container performs a category containment test (with negative result, of course) that is <em>faster</em> than a Python class check!
</p>
<p>
<strong><span class="underline">Testing rings</span></strong>
</p>
<p>
As you have observed, the current <code>is_Ring</code> function is suboptimal. I rewrote it in <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/10667" title="enhancement: Morphisms and Objects of Categories (needs_work)">#10667</a>.
</p>
<p>
Without <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/10667" title="enhancement: Morphisms and Objects of Categories (needs_work)">#10667</a> (but with <a class="closed ticket" href="https://trac.sagemath.org/ticket/11115" title="enhancement: Rewrite cached_method in Cython (closed: fixed)">#11115</a> for a fast cache):
</p>
<pre class="wiki">sage: from sage.rings.ring import is_Ring
sage: MS = MatrixSpace(QQ,2)
sage: %timeit is_Ring(QQ)
625 loops, best of 3: 5.1 µs per loop
sage: is_Ring(MS)
True
sage: %timeit is_Ring(MS)
625 loops, best of 3: 17.3 µs per loop
sage: C = Rings()
sage: %timeit QQ in C
625 loops, best of 3: 4.18 µs per loop
sage: %timeit MS in C
625 loops, best of 3: 4.31 µs per loop
</pre><p>
With <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/10667" title="enhancement: Morphisms and Objects of Categories (needs_work)">#10667</a> in addition:
</p>
<pre class="wiki">sage: from sage.rings.ring import is_Ring
sage: MS = MatrixSpace(QQ,2)
sage: %timeit is_Ring(QQ)
625 loops, best of 3: 259 ns per loop
sage: %timeit is_Ring(MS)
625 loops, best of 3: 17.5 µs per loop
sage: C = Rings().objects()
sage: %timeit QQ in C
625 loops, best of 3: 1.49 µs per loop
sage: %timeit MS in C
625 loops, best of 3: 1.57 µs per loop
</pre>
TicketmderickxThu, 01 Sep 2011 00:57:18 GMTdependencies, description changed
https://trac.sagemath.org/ticket/9054#comment:48
https://trac.sagemath.org/ticket/9054#comment:48
<ul>
<li><strong>dependencies</strong>
changed from <em>#9094, #11034</em> to <em>#9094, #11751</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/9054?action=diff&version=48">diff</a>)
</li>
</ul>
TicketmderickxThu, 01 Sep 2011 00:59:28 GMTstatus changed
https://trac.sagemath.org/ticket/9054#comment:49
https://trac.sagemath.org/ticket/9054#comment:49
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
</ul>
<p>
Ok I'm done with my reviewing of the original work. I guess a review patch of 39.8 KB deserves a review of its own :P
</p>
TicketmderickxThu, 01 Sep 2011 02:53:29 GMT
https://trac.sagemath.org/ticket/9054#comment:50
https://trac.sagemath.org/ticket/9054#comment:50
<p>
Note that this patch needs the patch at <a class="ext-link" href="http://trac.sagemath.org/sage_trac/ticket/11751"><span class="icon"></span>http://trac.sagemath.org/sage_trac/ticket/11751</a> to work, but altough the patch at that ticket makes all the doctest for function fields pass, it makes a lot of other doctests fail :(
</p>
TicketmderickxSat, 10 Sep 2011 21:21:15 GMT
https://trac.sagemath.org/ticket/9054#comment:51
https://trac.sagemath.org/ticket/9054#comment:51
<p>
Ok <a class="closed ticket" href="https://trac.sagemath.org/ticket/11751" title="defect: make free_module_generic_pid also work for pid's other than integers (closed: fixed)">#11751</a> is ready for review and the code here passes all tests (at least I tested it on sage 4.7.2.alpha2 ) after you apply the tickets at 11751. So this one can finally get merged as soon as it has positive review.
</p>
TicketmderickxSun, 11 Sep 2011 09:15:34 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054-review.patch</em>
</li>
</ul>
TicketmderickxSun, 11 Sep 2011 09:19:14 GMTdependencies, description changed
https://trac.sagemath.org/ticket/9054#comment:52
https://trac.sagemath.org/ticket/9054#comment:52
<ul>
<li><strong>dependencies</strong>
changed from <em>#9094, #11751</em> to <em>#9094, #11751, #9138</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/9054?action=diff&version=52">diff</a>)
</li>
</ul>
TicketmderickxSun, 11 Sep 2011 09:35:42 GMTdescription changed
https://trac.sagemath.org/ticket/9054#comment:53
https://trac.sagemath.org/ticket/9054#comment:53
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9054?action=diff&version=53">diff</a>)
</li>
</ul>
TicketsaraedumWed, 14 Sep 2011 15:47:13 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054_undo_unittest.patch</em>
</li>
</ul>
<p>
revert changes to misc.unittest introduced by the review patch
</p>
TicketmderickxWed, 14 Sep 2011 20:25:59 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054-invert_ideal.patch</em>
</li>
</ul>
TicketsaraedumThu, 15 Sep 2011 00:54:34 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054_isFunctionField.patch</em>
</li>
</ul>
<p>
use category in is_FunctionField()
</p>
TicketsaraedumThu, 15 Sep 2011 01:04:28 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054_cached_method.patch</em>
</li>
</ul>
<p>
replace manual caching with cached_method
</p>
TicketsaraedumThu, 15 Sep 2011 01:11:43 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054_maximal_order_member_check.patch</em>
</li>
</ul>
<p>
_element_constructor_ checks that element is in maximal order
</p>
TicketsaraedumThu, 15 Sep 2011 01:13:37 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054_call_super_constructors.patch</em>
</li>
</ul>
<p>
added missing calls to superclass constructors
</p>
TicketsaraedumThu, 15 Sep 2011 01:17:44 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054_UniqueFactory.patch</em>
</li>
</ul>
<p>
use <a class="missing wiki">UniqueFactory?</a> instead of cached_method in constructors
</p>
TicketsaraedumThu, 15 Sep 2011 01:21:21 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054_maps_refactor.patch</em>
</li>
</ul>
<p>
refactored maps class hieararchy
</p>
TicketsaraedumThu, 15 Sep 2011 02:04:50 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054_doctests-3.patch</em>
</li>
</ul>
<p>
extended and unified doctests
</p>
TicketsaraedumThu, 15 Sep 2011 02:07:18 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054_cleanup.patch</em>
</li>
</ul>
<p>
cleanup code and imports
</p>
TicketsaraedumThu, 15 Sep 2011 02:12:19 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054_authors.patch</em>
</li>
</ul>
<p>
added authors and copyright headers
</p>
TicketsaraedumThu, 15 Sep 2011 02:18:19 GMTmilestone, description, author changed; reviewer set
https://trac.sagemath.org/ticket/9054#comment:54
https://trac.sagemath.org/ticket/9054#comment:54
<ul>
<li><strong>reviewer</strong>
set to <em>Maarten Derickx, Julian Rueth</em>
</li>
<li><strong>milestone</strong>
changed from <em>sage-wishlist</em> to <em>sage-4.7.2</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/9054?action=diff&version=54">diff</a>)
</li>
<li><strong>author</strong>
changed from <em>William Stein, Robert Bradshaw, Maarten Derickx, Moritz Minzlaff</em> to <em>William Stein, Robert Bradshaw, Maarten Derickx, Moritz Minzlaff, Julian Rueth</em>
</li>
</ul>
TicketsaraedumThu, 15 Sep 2011 02:20:54 GMTdescription changed
https://trac.sagemath.org/ticket/9054#comment:55
https://trac.sagemath.org/ticket/9054#comment:55
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9054?action=diff&version=55">diff</a>)
</li>
</ul>
<p>
Apply trac_9054-all-parts.patch, trac_9054_polynomial_base_field.patch, trac_9054_zero.patch, trac_9054_codomain.patch, trac_9054_doctest-2.patch, trac_9054-review.patch, trac_9054_undo_unittest.patch, trac_9054-invert_ideal.patch, trac_9054_isFunctionField.patch, trac_9054_UniqueFactory.patch, trac_9054_cached_method.patch, trac_9054_maximal_order_member_check.patch, trac_9054_call_super_constructors.patch, trac_9054_maps_refactor.patch, trac_9054_doctests-3.patch, trac_9054_cleanup.patch, trac_9054_authors.patch
</p>
TicketsaraedumThu, 15 Sep 2011 02:36:56 GMT
https://trac.sagemath.org/ticket/9054#comment:56
https://trac.sagemath.org/ticket/9054#comment:56
<p>
(Apparently the patchbot expects these "Apply" instructions in a comment and not in the ticket description)
</p>
<p>
A more detailed description of the patches since <code>trac_9054-invert_ideal.patch</code>:
</p>
<ul><li><code>trac_9054_isFunctionField.patch</code> hopefully does what Simon King proposed for <code>is_FunctionField</code>
</li><li><code>trac_9054_UniqueFactory.patch</code> replaces the <code>@cached_method</code> in <code>constructor.py</code> with <a class="missing wiki">UniqueFactories?</a> -- apparently that class is meant for that purpose
</li><li><code>trac_9054_cached_method.patch</code> replaces all manual caching with <code>@cached_method</code> methods
</li><li><code>trac_9054_maximal_order_member_check.patch</code> fixes a todo about checking that members passed to an <code>_element_constructor</code> are actually in the order
</li><li><code>trac_9054_call_super_constructors.patch</code> is the one I'm not sure about. At two places the super classes were not properly called -- was that by intention? I hope this fixes it.
</li><li><code>trac_9054_maps_refactor.patch</code> slightly changes the base classes of function field morphisms
</li><li><code>trac_9054_doctests-3.patch</code> essentially unifies the naming of variables in the doctests, so function fields are now called K and L, variables x, y, z. Also I added an entry to <code>/doc/en/reference/index.rst</code>, is that correct?
</li><li><code>trac_9054_cleanup.patch</code> reorganizes some imports and removes empty lines
</li><li><code>trac_9054_authors.patch</code> adds authors and copyrights to the files. I followed <a href="http://www.sagemath.org/doc/developer/conventions.html#headings-of-sage-library-code-files">http://www.sagemath.org/doc/developer/conventions.html#headings-of-sage-library-code-files</a>, hopefully I got it right?
</li></ul><p>
I also reviewed Maarten's changes and they looked good except for the very few things I patched here. Maarten could you review my patches? It looks like a lot of work, but it should be fairly trivial to review.
</p>
TicketsaraedumMon, 19 Sep 2011 14:02:42 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054_is_function_field.patch</em>
</li>
</ul>
<p>
identical to trac_9054_isFunctionField.patch but the patch bot does not like upper case in patch files
</p>
TicketsaraedumMon, 19 Sep 2011 14:04:11 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054_unique_factory.patch</em>
</li>
</ul>
<p>
identical to trac_9054_UniqueFactory.patch (patchbot does not like uppercase)
</p>
TicketsaraedumMon, 19 Sep 2011 14:05:06 GMT
https://trac.sagemath.org/ticket/9054#comment:57
https://trac.sagemath.org/ticket/9054#comment:57
<p>
Apply trac_9054-all-parts.patch, trac_9054_polynomial_base_field.patch, trac_9054_zero.patch, trac_9054_codomain.patch, trac_9054_doctest-2.patch, trac_9054-review.patch, trac_9054_undo_unittest.patch, trac_9054-invert_ideal.patch, trac_9054_is_function_field.patch, trac_9054_unique_factory.patch, trac_9054_cached_method.patch, trac_9054_maximal_order_member_check.patch, trac_9054_call_super_constructors.patch, trac_9054_maps_refactor.patch, trac_9054_doctests-3.patch, trac_9054_cleanup.patch, trac_9054_authors.patch
</p>
TicketsaraedumMon, 19 Sep 2011 15:25:00 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054_reference.patch</em>
</li>
</ul>
<p>
fixes in the reference manual
</p>
TicketsaraedumMon, 19 Sep 2011 15:29:53 GMTdescription changed
https://trac.sagemath.org/ticket/9054#comment:58
https://trac.sagemath.org/ticket/9054#comment:58
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9054?action=diff&version=58">diff</a>)
</li>
</ul>
TicketsaraedumTue, 20 Sep 2011 09:41:20 GMTstatus changed
https://trac.sagemath.org/ticket/9054#comment:59
https://trac.sagemath.org/ticket/9054#comment:59
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
<a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9054/trac_9054_cleanup.patch" title="Attachment 'trac_9054_cleanup.patch' in Ticket #9054">trac_9054_cleanup.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9054/trac_9054_cleanup.patch" title="Download"></a> introduced a problem with cyclic imports — I'm working on it.
</p>
TicketsaraedumTue, 20 Sep 2011 12:30:39 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054_factor.patch</em>
</li>
</ul>
<p>
fixes an import problem in factor()
</p>
TicketsaraedumTue, 20 Sep 2011 12:32:07 GMTdescription changed
https://trac.sagemath.org/ticket/9054#comment:60
https://trac.sagemath.org/ticket/9054#comment:60
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9054?action=diff&version=60">diff</a>)
</li>
</ul>
<p>
It turned out not to be a cyclic import problem but just the wrong module that was imported. I'm waiting for the doctests to set this back to needs_review.
</p>
TicketsaraedumTue, 20 Sep 2011 13:27:02 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054_order_category.patch</em>
</li>
</ul>
<p>
orders have no category set
</p>
TicketsaraedumTue, 20 Sep 2011 13:48:04 GMTstatus, description changed
https://trac.sagemath.org/ticket/9054#comment:61
https://trac.sagemath.org/ticket/9054#comment:61
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/9054?action=diff&version=61">diff</a>)
</li>
</ul>
TicketmderickxSat, 05 Nov 2011 18:01:25 GMTstatus changed
https://trac.sagemath.org/ticket/9054#comment:62
https://trac.sagemath.org/ticket/9054#comment:62
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
On sage.math using the just released sage-4.7.2 with the following 21! patches applied
</p>
<pre class="wiki">mderickx@sage:/scratch/mderickx/sage/devel/sage$ hg qser | nl
1 9138_flat.patch
2 trac_9054-all-parts.patch
3 trac_9054_polynomial_base_field.patch
4 trac_9054_zero.patch
5 trac_9054_codomain.patch
6 trac_9054_doctest-2.patch
7 trac_9054-review.patch
8 trac_9054_undo_unittest.patch
9 trac_9054-invert_ideal.patch
10 trac_9054_is_function_field.patch
11 trac_9054_unique_factory.patch
12 trac_9054_cached_method.patch
13 trac_9054_maximal_order_member_check.patch
14 trac_9054_call_super_constructors.patch
15 trac_9054_maps_refactor.patch
16 trac_9054_doctests-3.patch
17 trac_9054_cleanup.patch
18 trac_9054_authors.patch
19 trac_9054_reference.patch
20 trac_9054_factor.patch
21 trac_9054_order_category.patch
</pre><p>
I get
</p>
<pre class="wiki">
The following tests failed:
sage -t --long devel/sage-main/sage/rings/function_field/maps.py # 1 doctests failed
sage -t --long devel/sage-main/sage/rings/function_field/function_field.py # 7 doctests failed
----------------------------------------------------------------------
Total time for all tests: 1102.4 seconds
</pre>
TicketmderickxSat, 05 Nov 2011 18:03:14 GMTstatus changed
https://trac.sagemath.org/ticket/9054#comment:63
https://trac.sagemath.org/ticket/9054#comment:63
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
Sorry false alarm. I didn't have all patches applied!
</p>
TicketmderickxMon, 07 Nov 2011 13:36:35 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054-all-parts.patch</em>
</li>
</ul>
<p>
All patches till review.patch combined
</p>
TicketmderickxMon, 07 Nov 2011 13:37:13 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054-julian-combined.patch</em>
</li>
</ul>
<p>
All julians patches after review.patch combined
</p>
TicketmderickxMon, 07 Nov 2011 13:37:58 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054-review2.patch</em>
</li>
</ul>
<p>
Fixes last minor errors introduced by julians patches
</p>
TicketmderickxMon, 07 Nov 2011 13:45:47 GMTdescription changed
https://trac.sagemath.org/ticket/9054#comment:64
https://trac.sagemath.org/ticket/9054#comment:64
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9054?action=diff&version=64">diff</a>)
</li>
</ul>
<p>
It turned out that also when applying all julians patches to sage 4.7.2 with <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a> we get some errors. I fixed this in my minor review2.patch. I also combined some patches so that it becomes easier for someone else to do something with this ticket (i.e. doesnt have to download 20 patches). I'm now reading trough <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9054/trac_9054-julian-combined.patch" title="Attachment 'trac_9054-julian-combined.patch' in Ticket #9054">trac_9054-julian-combined.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9054/trac_9054-julian-combined.patch" title="Download"></a> if it does logical things.
</p>
TicketmderickxMon, 07 Nov 2011 14:08:50 GMTdescription changed
https://trac.sagemath.org/ticket/9054#comment:65
https://trac.sagemath.org/ticket/9054#comment:65
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9054?action=diff&version=65">diff</a>)
</li>
</ul>
TicketSimonKingMon, 07 Nov 2011 14:32:54 GMT
https://trac.sagemath.org/ticket/9054#comment:66
https://trac.sagemath.org/ticket/9054#comment:66
<p>
Just a note on <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a>: It had already been merged, but was unmerged because of an unacceptable regression in elliptic curve computations. But at <a class="closed ticket" href="https://trac.sagemath.org/ticket/11900" title="defect: Serious regression caused by #9138 (closed: fixed)">#11900</a>, I was able to avoid the regression and even turn it into a speed-up, in some cases. <a class="closed ticket" href="https://trac.sagemath.org/ticket/11900" title="defect: Serious regression caused by #9138 (closed: fixed)">#11900</a> needs review, and then I guess <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a> would be merged again.
</p>
TicketmderickxMon, 07 Nov 2011 15:58:31 GMTstatus changed
https://trac.sagemath.org/ticket/9054#comment:67
https://trac.sagemath.org/ticket/9054#comment:67
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Ok these are the results from reading trough you patches:
</p>
<p>
Why did you make some_elements in function_field.py return only one element? This number should be at least two (and preferable even at least 3) since else a lot of tests in <a class="missing wiki">TestSuite?</a>(F).run() will be meaningless with just one element because one element is always equal to itself for example!
</p>
<p>
If you make vector_space a cached method then why don't you change
</p>
<pre class="wiki">self._vector_space = (V, from_V, to_V)
return self._vector_space
</pre><p>
to
</p>
<pre class="wiki">return (V, from_V, to_V)
</pre><p>
This code is in two places.
</p>
<p>
In function_field_order.py there is a typo in the sentence "the function field in which this iss an order."
</p>
<p>
Why did you remove:
</p>
<pre class="wiki">if is_Ideal(gens):
gens = gens.gens()
</pre><p>
in function_field_order.py. I suspect the code was there to make the (not doctested) use case of:
</p>
<pre class="wiki">sage: K.<x> = FunctionField(QQ)
sage: O=K.maximal_order()
sage: I=O.ideal(x)
sage: O.ideal(I)
</pre><p>
since you should be able to make an ideal with input an ideal.
</p>
<p>
For the rest your combination patch looks very nice. Also good that you made the documentation quality so much higher. If you either answer the above questions with the right arguments or if you change them back it seems that we can finally have function fields in sage!
</p>
TicketsaraedumMon, 07 Nov 2011 16:18:00 GMT
https://trac.sagemath.org/ticket/9054#comment:68
https://trac.sagemath.org/ticket/9054#comment:68
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9054#comment:67" title="Comment 67">mderickx</a>:
</p>
<blockquote class="citation">
<p>
Why did you make some_elements in function_field.py return only one element? This number should be at least two (and preferable even at least 3) since else a lot of tests in <a class="missing wiki">TestSuite?</a>(F).run() will be meaningless with just one element because one element is always equal to itself for example!
</p>
</blockquote>
<p>
I think I had seen that somewhere else only one element was returned and copied that. (at that time I didn't know what some_elements() was good for)
I'll fix that.
</p>
<blockquote class="citation">
<p>
If you make vector_space a cached method then why don't you change
</p>
<pre class="wiki">self._vector_space = (V, from_V, to_V)
return self._vector_space
</pre><p>
to
</p>
<pre class="wiki">return (V, from_V, to_V)
</pre><p>
This code is in two places.
</p>
</blockquote>
<p>
That's true. Must have missed that.
</p>
<blockquote class="citation">
<p>
In function_field_order.py there is a typo in the sentence "the function field in which this iss an order."
</p>
</blockquote>
<p>
Will be fixed in the next patch.
</p>
<blockquote class="citation">
<p>
Why did you remove:
</p>
<pre class="wiki">if is_Ideal(gens):
gens = gens.gens()
</pre><p>
in function_field_order.py. I suspect the code was there to make the (not doctested) use case of:
</p>
<pre class="wiki">sage: K.<x> = FunctionField(QQ)
sage: O=K.maximal_order()
sage: I=O.ideal(x)
sage: O.ideal(I)
</pre><p>
since you should be able to make an ideal with input an ideal.
</p>
</blockquote>
<p>
Good question. It's part of a doctest patch so I guess it just got in by accident.
</p>
<blockquote class="citation">
<p>
For the rest your combination patch looks very nice. Also good that you made the documentation quality so much higher. If you either answer the above questions with the right arguments or if you change them back it seems that we can finally have function fields in sage!
</p>
</blockquote>
<p>
Ok. I'll prepare a patch to fix these issues. Thanks you took the time and had a look at these patches. :)
</p>
TicketsaraedumMon, 07 Nov 2011 19:58:18 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054_review_fixup.patch</em>
</li>
</ul>
<p>
patches to mderickx's review comments
</p>
TicketsaraedumMon, 07 Nov 2011 20:07:51 GMTstatus, description changed
https://trac.sagemath.org/ticket/9054#comment:69
https://trac.sagemath.org/ticket/9054#comment:69
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/9054?action=diff&version=69">diff</a>)
</li>
</ul>
<p>
Apply trac_9054-all-parts.patch, trac_9054-julian-combined.patch, trac_9054-review2.patch, trac_9054_review_fixup.patch.
</p>
<p>
Maarten, I'm not so sure about the is_Ideal() check anymore. Is it really expected behavior that ideal(I) creates the ideal generated by the generators of I — no matter where the ideal I lives? If you feel like that should happen, then add these two lines again and set the ticket to positive review. Or don't add them if you feel that people should be more explicit by actually calling ideal(I.gens()).
</p>
TicketmderickxMon, 07 Nov 2011 23:20:31 GMT
https://trac.sagemath.org/ticket/9054#comment:70
https://trac.sagemath.org/ticket/9054#comment:70
<p>
I will add it just to be consistent with numberfields.
</p>
<pre class="wiki">sage: K.<a> = QQ.extension(x^2-2)
sage: I = K.ideal(3)
sage: L.<b> = K.extension(x^2-3)
sage: L.ideal(I)
Fractional ideal (3)
sage: L.ideal(p).factor()
(Fractional ideal (b))^2
</pre><p>
Note that it also mathematically makes sense in the most general setting since the ideal created this way is the ideal extension corresponding to the coersion map from I.ring() to self.
</p>
TicketmderickxTue, 08 Nov 2011 00:31:52 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054-can_this_really_be_the_last.patch</em>
</li>
</ul>
TicketmderickxTue, 08 Nov 2011 00:33:09 GMTdescription changed
https://trac.sagemath.org/ticket/9054#comment:71
https://trac.sagemath.org/ticket/9054#comment:71
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9054?action=diff&version=71">diff</a>)
</li>
</ul>
TicketmderickxTue, 08 Nov 2011 00:34:32 GMT
https://trac.sagemath.org/ticket/9054#comment:72
https://trac.sagemath.org/ticket/9054#comment:72
<p>
If you can just check my last patch then it can have positive review.
</p>
TicketsaraedumTue, 08 Nov 2011 16:32:04 GMTdescription changed
https://trac.sagemath.org/ticket/9054#comment:73
https://trac.sagemath.org/ticket/9054#comment:73
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9054?action=diff&version=73">diff</a>)
</li>
</ul>
<pre class="wiki">sage: K.<x> = FunctionField(QQ)
sage: R.<y> = K[]
sage: L.<y> = K.extension(y^3-x)
sage: loads(dumps(L))
AttributeError: ("'module' object has no attribute 'FunctionField_polymod'", <built-in function lookup_global>, ('FunctionField_polymod',))
</pre><p>
This was also checked by <code>sage: TestSuite(L).run() #long time</code> in function_field.py.
</p>
<p>
The latest patch fixes this problem.
</p>
<p>
Maarten, if you agree with this latest patch you can set it to positive review.
</p>
TicketmderickxTue, 08 Nov 2011 17:08:48 GMT
https://trac.sagemath.org/ticket/9054#comment:74
https://trac.sagemath.org/ticket/9054#comment:74
<p>
I guess my last ticket name was a bit to hopefull. I just forgot to do add a --long after sage -tp 20 once and immediately a bug slips trough. I'm now testing everything with your last patch.
</p>
TicketmderickxTue, 08 Nov 2011 20:14:37 GMT
https://trac.sagemath.org/ticket/9054#comment:75
https://trac.sagemath.org/ticket/9054#comment:75
<p>
One more question. Shouldn't the line
</p>
<p>
<a class="missing wiki">FunctionField?</a> = <a class="missing wiki">FunctionFieldFactory?</a>("<a class="missing wiki">FunctionField?</a>")
</p>
<p>
also be changed in a way similar in you last patch. I mean the two things should work in the same way right?
</p>
TicketsaraedumWed, 09 Nov 2011 12:46:45 GMT
https://trac.sagemath.org/ticket/9054#comment:76
https://trac.sagemath.org/ticket/9054#comment:76
<p>
We could change it but it is not necessary. <code>FunctionField</code> is exported to sage.all so the pickling infrastructure can find the name there. <code>FunctionField_polymod</code>, however, can not be found in sage.all, that's why there is the fully qualified name.
</p>
TicketmderickxFri, 11 Nov 2011 12:19:55 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054_pickling.patch</em>
</li>
</ul>
<p>
fix pickling of FunctionField_polymod
</p>
TicketmderickxFri, 11 Nov 2011 12:22:56 GMT
https://trac.sagemath.org/ticket/9054#comment:77
https://trac.sagemath.org/ticket/9054#comment:77
<p>
I'd like to have the consistency so I changed you last patch. If your ok with it this ticket can finally have a positive review.
</p>
TicketsaraedumFri, 11 Nov 2011 15:01:33 GMTstatus changed
https://trac.sagemath.org/ticket/9054#comment:78
https://trac.sagemath.org/ticket/9054#comment:78
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
TicketjdemeyerFri, 11 Nov 2011 22:23:00 GMTmilestone changed
https://trac.sagemath.org/ticket/9054#comment:79
https://trac.sagemath.org/ticket/9054#comment:79
<ul>
<li><strong>milestone</strong>
changed from <em>sage-4.8</em> to <em>sage-pending</em>
</li>
</ul>
TicketjdemeyerTue, 15 Nov 2011 11:36:39 GMTstatus changed
https://trac.sagemath.org/ticket/9054#comment:80
https://trac.sagemath.org/ticket/9054#comment:80
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
The commit messages of the patches could be cleaned up:
</p>
<ol><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9054/trac_9054-julian-combined.patch" title="Attachment 'trac_9054-julian-combined.patch' in Ticket #9054">trac_9054-julian-combined.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9054/trac_9054-julian-combined.patch" title="Download"></a>: the commit message <em>starts</em> with <code>* * *</code> instead of something useful.
</li><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9054/trac_9054-review.patch" title="Attachment 'trac_9054-review.patch' in Ticket #9054">trac_9054-review.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9054/trac_9054-review.patch" title="Download"></a> has no proper commit message. This improper commit message is also in <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9054/trac_9054-all-parts.patch" title="Attachment 'trac_9054-all-parts.patch' in Ticket #9054">trac_9054-all-parts.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9054/trac_9054-all-parts.patch" title="Download"></a>, which should be fixed.
</li><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9054/trac_9054-all-parts.patch" title="Attachment 'trac_9054-all-parts.patch' in Ticket #9054">trac_9054-all-parts.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9054/trac_9054-all-parts.patch" title="Download"></a> "<code>contains parts 1-12, marteen's additions and final doctest fixes</code>" makes no sense if you don't know this ticket, the message should makes sense on its own. The word "function field" does not even appear in the message of this patch!
</li></ol>
TicketsaraedumTue, 15 Nov 2011 13:36:28 GMT
https://trac.sagemath.org/ticket/9054#comment:81
https://trac.sagemath.org/ticket/9054#comment:81
<p>
I'm replacing the commit messages now. I don't have privileges to replace attachements so I have to upload a new set of patches instead.
</p>
TicketsaraedumTue, 15 Nov 2011 13:36:59 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054-all-parts.2.patch</em>
</li>
</ul>
<p>
provide basic function field arithmetic (combined patch by various authors)
</p>
TicketsaraedumTue, 15 Nov 2011 13:37:50 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054-julian-combined.2.patch</em>
</li>
</ul>
<p>
cleanup function field code and documentation
</p>
TicketsaraedumTue, 15 Nov 2011 13:38:18 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054-review2.2.patch</em>
</li>
</ul>
<p>
fix doctests for function fields
</p>
TicketsaraedumTue, 15 Nov 2011 13:38:47 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054_review_fixup.2.patch</em>
</li>
</ul>
<p>
fixes for function fields related to the review comments by mderickx
</p>
TicketsaraedumTue, 15 Nov 2011 13:39:14 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054-can_this_really_be_the_last.2.patch</em>
</li>
</ul>
<p>
last fixes for function fields
</p>
TicketsaraedumTue, 15 Nov 2011 13:39:44 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>trac_9054_pickling.2.patch</em>
</li>
</ul>
<p>
fix pickling for extensions of function fields
</p>
TicketsaraedumTue, 15 Nov 2011 13:42:17 GMTstatus, description changed
https://trac.sagemath.org/ticket/9054#comment:82
https://trac.sagemath.org/ticket/9054#comment:82
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>positive_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/9054?action=diff&version=82">diff</a>)
</li>
</ul>
<p>
Apply trac_9054-all-parts.2.patch, trac_9054-julian-combined.2.patch, trac_9054-review2.2.patch, trac_9054_review_fixup.2.patch, trac_9054-can_this_really_be_the_last.2.patch, trac_9054_pickling.2.patch
</p>
TicketjdemeyerFri, 09 Dec 2011 23:57:29 GMTmilestone changed
https://trac.sagemath.org/ticket/9054#comment:83
https://trac.sagemath.org/ticket/9054#comment:83
<ul>
<li><strong>milestone</strong>
changed from <em>sage-pending</em> to <em>sage-5.0</em>
</li>
</ul>
TicketjdemeyerSun, 22 Jan 2012 21:23:44 GMTstatus changed
https://trac.sagemath.org/ticket/9054#comment:84
https://trac.sagemath.org/ticket/9054#comment:84
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Patches <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9054/trac_9054-all-parts.2.patch" title="Attachment 'trac_9054-all-parts.2.patch' in Ticket #9054">trac_9054-all-parts.2.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9054/trac_9054-all-parts.2.patch" title="Download"></a> and <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9054/trac_9054-review2.2.patch" title="Attachment 'trac_9054-review2.2.patch' in Ticket #9054">trac_9054-review2.2.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9054/trac_9054-review2.2.patch" title="Download"></a> apply with fuzz 2 against sage-5.0.beta1. Please rebase such that they apply cleanly.
</p>
TicketjdemeyerMon, 30 Jan 2012 10:51:06 GMTstatus, dependencies, description changed
https://trac.sagemath.org/ticket/9054#comment:85
https://trac.sagemath.org/ticket/9054#comment:85
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>positive_review</em>
</li>
<li><strong>dependencies</strong>
changed from <em>#9094, #11751, #9138</em> to <em>sage-5.0.beta1</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/9054?action=diff&version=85">diff</a>)
</li>
</ul>
TicketmderickxMon, 30 Jan 2012 12:55:11 GMT
https://trac.sagemath.org/ticket/9054#comment:86
https://trac.sagemath.org/ticket/9054#comment:86
<p>
Thanks for rebasing, I added it to my todo list, but didn't get to it yet.
</p>
TicketjdemeyerTue, 31 Jan 2012 09:08:07 GMTattachment set
https://trac.sagemath.org/ticket/9054
https://trac.sagemath.org/ticket/9054
<ul>
<li><strong>attachment</strong>
set to <em>9054_function_fields.patch</em>
</li>
</ul>
TicketjdemeyerThu, 02 Feb 2012 12:52:09 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/9054#comment:87
https://trac.sagemath.org/ticket/9054#comment:87
<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-5.0.beta2</em>
</li>
</ul>
Ticket