Sage: Ticket #8327: Implement the universal cyclotomic field, using Zumbroich basis
https://trac.sagemath.org/ticket/8327
<p>
This patch provides the universal cyclotomic field
</p>
<pre class="wiki"> sage: UCF.<E> = UniversalCyclotomicField(); UCF
Universal Cyclotomic Field endowed with the Zumbroich basis
</pre><p>
in sage. This field is the smallest field extension of QQ which contains all roots of unity.
</p>
<pre class="wiki"> sage: E(3); E(3)^3
E(3)
1
sage: E(6); E(6)^2; E(6)^3; E(6)^6
-E(3)^2
E(3)
-1
1
</pre><p>
It comes equipped with a distinguished basis, called the Zumbroich
basis, which gives, for any n, A basis of QQ( E(n) ) over QQ, where (n,k) stands for E(n)<sup>k.
</sup></p>
<pre class="wiki"> sage: UCF.zumbroich_basis(6)
[(6, 2), (6, 4)]
</pre><p>
As seen for E(6), every element in UCF is expressed in terms of the smallest cyclotomic field in which it is contained.
</p>
<pre class="wiki">sage: E(6)*E(4)
-E(12)^11
</pre><p>
It provides arithmetics on UCF as addition, multiplication, and inverses:
</p>
<pre class="wiki"> sage: E(3)+E(4)
E(12)^4 - E(12)^7 - E(12)^11
sage: E(3)*E(4)
E(12)^7
sage: (E(3)+E(4)).inverse()
E(12)^4 + E(12)^8 + E(12)^11
sage: (E(3)+E(4))*(E(3)+E(4)).inverse()
1
</pre><p>
And also things like Galois conjugates.
</p>
<pre class="wiki"> sage: (E(3)+E(4)).galois_conjugates()
[E(12)^4 - E(12)^7 - E(12)^11, -E(12)^7 + E(12)^8 - E(12)^11, E(12)^4 + E(12)^7 + E(12)^11, E(12)^7 + E(12)^8 + E(12)^11]
</pre><p>
The ticket does not use the gap interface.
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/8327
Trac 1.1.6nthieryMon, 22 Feb 2010 17:49:31 GMTcc set
https://trac.sagemath.org/ticket/8327#comment:1
https://trac.sagemath.org/ticket/8327#comment:1
<ul>
<li><strong>cc</strong>
<em>sage-combinat</em> added
</li>
</ul>
TicketcraigcitroMon, 22 Feb 2010 19:03:44 GMT
https://trac.sagemath.org/ticket/8327#comment:2
https://trac.sagemath.org/ticket/8327#comment:2
<p>
So this seems interesting (I'd never heard of the Zumbrioch basis before). I don't have anything useful to say about the utility of this, or implementing it.
</p>
<p>
However, I'd like to suggest we might want to use a different convention for the constructor -- I don't like the idea of <code>CyclotomicField()</code> working. That seems like something that's too easy for a user to accidentally do (leave out the <code>n</code> they intended to use), and I'd much rather they see an error than have it quietly succeed, only to discover that their cyclotomic field isn't quite what they expected (and likely slower to boot). Maybe something like <code>F = CyclotomicField(n=Infinity)</code>?
</p>
TicketnthieryMon, 22 Feb 2010 21:46:41 GMT
https://trac.sagemath.org/ticket/8327#comment:3
https://trac.sagemath.org/ticket/8327#comment:3
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:2" title="Comment 2">craigcitro</a>:
</p>
<blockquote class="citation">
<p>
So this seems interesting (I'd never heard of the Zumbrioch basis before). I don't have anything useful to say about the utility of this, or implementing it.
</p>
<p>
However, I'd like to suggest we might want to use a different convention for the constructor -- I don't like the idea of <code>CyclotomicField()</code> working. That seems like something that's too easy for a user to accidentally do (leave out the <code>n</code> they intended to use), and I'd much rather they see an error than have it quietly succeed, only to discover that their cyclotomic field isn't quite what they expected (and likely slower to boot). Maybe something like <code>F = CyclotomicField(n=Infinity)</code>?
</p>
</blockquote>
<p>
I indeed take any better suggestion! In Gap that would be Cyclotomics, but it does sound good in Field in the name. n=infinity might be misleading, since it's not about infinite roots of unity.
</p>
TicketcraigcitroMon, 22 Feb 2010 21:48:39 GMT
https://trac.sagemath.org/ticket/8327#comment:4
https://trac.sagemath.org/ticket/8327#comment:4
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:3" title="Comment 3">nthiery</a>:
</p>
<blockquote class="citation">
<p>
I indeed take any better suggestion! In Gap that would be Cyclotomics, but it does sound good in Field in the name. n=infinity might be misleading, since it's not about infinite roots of unity.
</p>
</blockquote>
<p>
True. Maybe it's a bit too "cutesy," but using <code>n=0</code> might be nice -- after all, the <code>n</code>th cyclotomic field has roots of unity for all divisors of <code>n</code>, so this would still hold for the universal cyclotomic field and <code>n=0</code>.
</p>
TicketnthieryMon, 08 Mar 2010 14:11:14 GMT
https://trac.sagemath.org/ticket/8327#comment:5
https://trac.sagemath.org/ticket/8327#comment:5
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:4" title="Comment 4">craigcitro</a>:
</p>
<blockquote class="citation">
<p>
True. Maybe it's a bit too "cutesy," but using <code>n=0</code> might be nice -- after all, the <code>n</code>th cyclotomic field has roots of unity for all divisors of <code>n</code>, so this would still hold for the universal cyclotomic field and <code>n=0</code>.
</p>
</blockquote>
<p>
Mmm, any complex number is a 0-th root of unity, isn't it?
</p>
Ticketstumpc5Fri, 04 Jun 2010 13:04:33 GMTstatus changed
https://trac.sagemath.org/ticket/8327#comment:6
https://trac.sagemath.org/ticket/8327#comment:6
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_work</em>
</li>
</ul>
<p>
Does anyone have a pdf copy of Zumbroich's thesis where the algorithms are described?
</p>
<p>
There is a nice version of a modified Zumbroich basis in the article I used (both are implemented in the attached file) but I don't see how the author expresses any root of unity in terms of the basis (probably just due to my lack of knowledge). But Zumbroich should describe it in more detail in his thesis.
</p>
Ticketstumpc5Mon, 07 Jun 2010 01:50:37 GMT
https://trac.sagemath.org/ticket/8327#comment:7
https://trac.sagemath.org/ticket/8327#comment:7
<p>
The attached class <a class="missing wiki">NewCyclotomicField?</a> uses the modified Zumbroich basis and behaves already somehow as it should (beside pretty printing).
</p>
<p>
If people think the Zumbroich basis itself is nicer, it's easy to change. I personally think the modified version as defined in Breuer - "Integral bases for subfields of cyclotomic fields" AAECC8, 279--289 (1997) has nicer properties (see the definition of the set S_n and Remark 1).
</p>
<p>
Here are some implementation problems I have:
</p>
<ul><li>how can I replace <a class="missing wiki">CombinatorialAlgebra?</a> by a new field constructor (I haven't found a description how to define a field)?
</li></ul><ul><li>how can I embed a <a class="missing wiki">CombinatorialAlgebra?</a> into another one (here: <a class="missing wiki">NewCyclotomicField?</a>(m) into <a class="missing wiki">NewCyclotomicField?</a>(n) for m | n)?
</li></ul><ul><li>how can I define a new class <a class="missing wiki">UniversalCyclotomicField?</a> containing all cyclotomic fields?
</li></ul><ul><li>how can I define the attribute self._one in the constructor of <a class="missing wiki">NewCyclotomicField?</a> properly?
</li></ul>
TicketcwittyTue, 13 Jul 2010 21:48:21 GMTcc changed
https://trac.sagemath.org/ticket/8327#comment:8
https://trac.sagemath.org/ticket/8327#comment:8
<ul>
<li><strong>cc</strong>
<em>cwitty</em> added
</li>
</ul>
Ticketstumpc5Sun, 19 Sep 2010 23:55:42 GMTstatus changed
https://trac.sagemath.org/ticket/8327#comment:9
https://trac.sagemath.org/ticket/8327#comment:9
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
Ticketstumpc5Sun, 19 Sep 2010 23:56:22 GMTauthor set
https://trac.sagemath.org/ticket/8327#comment:10
https://trac.sagemath.org/ticket/8327#comment:10
<ul>
<li><strong>author</strong>
set to <em>Christian Stump</em>
</li>
</ul>
Ticketstumpc5Sun, 17 Oct 2010 14:40:25 GMTdescription changed
https://trac.sagemath.org/ticket/8327#comment:11
https://trac.sagemath.org/ticket/8327#comment:11
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/8327?action=diff&version=11">diff</a>)
</li>
</ul>
TicketdavidloefflerWed, 19 Jan 2011 15:54:12 GMTstatus changed; milestone set
https://trac.sagemath.org/ticket/8327#comment:12
https://trac.sagemath.org/ticket/8327#comment:12
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>milestone</strong>
set to <em>sage-feature</em>
</li>
</ul>
<p>
Apply trac_8327_universal_cyclotomic_field-cs.2.patch
</p>
<p>
(for patchbot -- I hope that's right!).
</p>
<p>
This doesn't quite look ready to me. I haven't tried applying the patch, but just on a visual check, I can see that lots of methods aren't documented or have no tests (*every* function added to Sage must be doctested, even those whose entire body is <code>"raise NotImplementedError"</code>). Also, the new module should be added to the reference manual, and the formatting should be valid ReST markup.
</p>
<p>
I very much hope you can get this code into shape -- it'd be a great thing to have in Sage -- but for now I'm afraid it's a thumbs down.
</p>
TicketdavidloefflerWed, 19 Jan 2011 15:56:19 GMTwork_issues set
https://trac.sagemath.org/ticket/8327#comment:13
https://trac.sagemath.org/ticket/8327#comment:13
<ul>
<li><strong>work_issues</strong>
set to <em>Won't build, needs more doctests and Sphinxified docstrings</em>
</li>
</ul>
<p>
I've looked at the patchbot logs, and it gets worse -- the Cython code isn't valid apparently.
</p>
Ticketstumpc5Wed, 26 Jan 2011 03:15:26 GMT
https://trac.sagemath.org/ticket/8327#comment:14
https://trac.sagemath.org/ticket/8327#comment:14
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:13" title="Comment 13">davidloeffler</a>:
</p>
<blockquote class="citation">
<p>
I've looked at the patchbot logs, and it gets worse -- the Cython code isn't valid apparently.
</p>
</blockquote>
<p>
This was only as cython booleans changed in the meantime. Beside that, the code was always working properly. I changed this minor problem, and it should be working now again.
</p>
<p>
I improved the documentation but there are still some things to be done...
</p>
Ticketstumpc5Tue, 22 Feb 2011 18:08:23 GMTstatus changed
https://trac.sagemath.org/ticket/8327#comment:15
https://trac.sagemath.org/ticket/8327#comment:15
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
For the buildbot:
</p>
<p>
Apply trac_8327_universal_cyclotomic_field-cs.patch
</p>
<p>
All other files are obsolete, but I cannot delete them. Some documentation is still missing, but only for methods which are helper functions for others.
</p>
<p>
Would be nice to get some feedback!
</p>
Ticketstumpc5Thu, 24 Feb 2011 17:39:35 GMT
https://trac.sagemath.org/ticket/8327#comment:16
https://trac.sagemath.org/ticket/8327#comment:16
<p>
Can some delete all but the youngest attached patch, the buildbot gets confused.
</p>
<p>
Thanks, Christian
</p>
Ticketstumpc5Sun, 13 Mar 2011 17:46:15 GMTdescription changed
https://trac.sagemath.org/ticket/8327#comment:17
https://trac.sagemath.org/ticket/8327#comment:17
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/8327?action=diff&version=17">diff</a>)
</li>
</ul>
TicketdavidloefflerFri, 18 Mar 2011 12:19:50 GMT
https://trac.sagemath.org/ticket/8327#comment:18
https://trac.sagemath.org/ticket/8327#comment:18
<p>
Apply trac_8327_universal_cyclotomic_field-cs.patch
</p>
Ticketstumpc5Wed, 23 Mar 2011 22:13:45 GMT
https://trac.sagemath.org/ticket/8327#comment:19
https://trac.sagemath.org/ticket/8327#comment:19
<p>
Apply only trac_8327_universal_cyclotomic_field-cs.patch
</p>
Ticketstumpc5Mon, 18 Apr 2011 18:53:06 GMTwork_issues changed
https://trac.sagemath.org/ticket/8327#comment:20
https://trac.sagemath.org/ticket/8327#comment:20
<ul>
<li><strong>work_issues</strong>
changed from <em>Won't build, needs more doctests and Sphinxified docstrings</em> to <em>needs more doctests and Sphinxified docstrings</em>
</li>
</ul>
<p>
Apply only trac_8327_universal_cyclotomic_field-cs.patch
</p>
Ticketstumpc5Sun, 24 Apr 2011 19:22:22 GMT
https://trac.sagemath.org/ticket/8327#comment:21
https://trac.sagemath.org/ticket/8327#comment:21
<p>
Apply only trac_8327_universal_cyclotomic_field-cs.patch
</p>
TicketchapotonFri, 10 Jun 2011 10:13:32 GMT
https://trac.sagemath.org/ticket/8327#comment:22
https://trac.sagemath.org/ticket/8327#comment:22
<p>
Apply trac_8327_universal_cyclotomic_field-cs.patch
</p>
Ticketstumpc5Fri, 10 Jun 2011 11:52:30 GMTdependencies set; owner deleted
https://trac.sagemath.org/ticket/8327#comment:23
https://trac.sagemath.org/ticket/8327#comment:23
<ul>
<li><strong>owner</strong>
changed from <em>davidloeffler</em> to <em>(none)</em>
</li>
<li><strong>dependencies</strong>
set to <em>#10963</em>
</li>
</ul>
Ticketstumpc5Sat, 11 Jun 2011 11:42:06 GMTmilestone changed
https://trac.sagemath.org/ticket/8327#comment:24
https://trac.sagemath.org/ticket/8327#comment:24
<ul>
<li><strong>milestone</strong>
changed from <em>sage-feature</em> to <em>sage-4.7.2</em>
</li>
</ul>
Ticketstumpc5Tue, 11 Oct 2011 13:39:17 GMTdependencies deleted
https://trac.sagemath.org/ticket/8327#comment:25
https://trac.sagemath.org/ticket/8327#comment:25
<ul>
<li><strong>dependencies</strong>
<em>#10963</em> deleted
</li>
</ul>
TicketnthieryTue, 15 Nov 2011 09:28:09 GMTstatus changed
https://trac.sagemath.org/ticket/8327#comment:26
https://trac.sagemath.org/ticket/8327#comment:26
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
When applying the sage-combinat queue on sage 4.8.alpha0, we got a compilation error on universal_cyclotomic_field_c.pyx, most likely due to the updated cython (<a class="closed ticket" href="https://trac.sagemath.org/ticket/11761" title="enhancement: Upgrade Cython to 0.15.1 (closed: fixed)">#11761</a>)
</p>
TicketSimonKingTue, 15 Nov 2011 20:40:54 GMTstatus, author changed
https://trac.sagemath.org/ticket/8327#comment:27
https://trac.sagemath.org/ticket/8327#comment:27
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>author</strong>
changed from <em>Christian Stump</em> to <em>Christian Stump, Simon King</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:26" title="Comment 26">nthiery</a>:
</p>
<blockquote class="citation">
<p>
When applying the sage-combinat queue on sage 4.8.alpha0, we got a compilation error on universal_cyclotomic_field_c.pyx, most likely due to the updated cython (<a class="closed ticket" href="https://trac.sagemath.org/ticket/11761" title="enhancement: Upgrade Cython to 0.15.1 (closed: fixed)">#11761</a>)
</p>
</blockquote>
<p>
My patch seems to fix the problem. At least, when one uses the new Cython spkg and applies the combinat queue up to and including Christian's patch and adds my patch as well (which is also in the combinat queue, I think), then sage builds and starts.
</p>
<p>
Note that the my patch does not add the new Cython as a dependency - it should run with the old Cython as well.
</p>
<p>
However, I already get a doctest error in <code>devel/sage-combinat/doc/en/thematic_tutorials/sandpile.rst</code>. Now it is needed to test whether this error is due to the patches from here, or due to some other patch in the combinat queue.
</p>
<p>
Since it is not sure whether THIS ticket is the culprit, and since it builds with the new cython, I am promiting it to "needs review".
</p>
<p>
Apply trac_8327_universal_cyclotomic_field-cs.patch trac_8327_universal_cyclotomic_field_new_cython-sk.patch
</p>
TicketSimonKingTue, 15 Nov 2011 20:43:28 GMTdescription changed
https://trac.sagemath.org/ticket/8327#comment:28
https://trac.sagemath.org/ticket/8327#comment:28
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/8327?action=diff&version=28">diff</a>)
</li>
</ul>
<p>
Sorry, I forgot to change the ticket description.
</p>
TicketSimonKingTue, 15 Nov 2011 20:50:05 GMTstatus, work_issues changed
https://trac.sagemath.org/ticket/8327#comment:29
https://trac.sagemath.org/ticket/8327#comment:29
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>work_issues</strong>
changed from <em>needs more doctests and Sphinxified docstrings</em> to <em>needs more doctests and Sphinxified docstrings, rebase first patch</em>
</li>
</ul>
<p>
Note that the first patch does not apply on a clean sage-4.8.alpha0: The hunk that adds <code>from sage.misc.lazy_import import lazy_import</code> to sage/categories/all.py does not match.
</p>
<p>
But is importing lazy_import really needed? If I see that correctly, it is not used in sage/categories/all.py
</p>
TicketSimonKingTue, 15 Nov 2011 20:52:38 GMTwork_issues changed
https://trac.sagemath.org/ticket/8327#comment:30
https://trac.sagemath.org/ticket/8327#comment:30
<ul>
<li><strong>work_issues</strong>
changed from <em>needs more doctests and Sphinxified docstrings, rebase first patch</em> to <em>Find dependency. Needs more doctests and Sphinxified docstrings.</em>
</li>
</ul>
<p>
Gosh. And my patch fails to apply as well, in both hunks.
</p>
<p>
So, apparently there is a dependency in the combinat queue. It should be named as a dependency for this ticket.
</p>
TicketSimonKingTue, 15 Nov 2011 20:56:03 GMTwork_issues changed
https://trac.sagemath.org/ticket/8327#comment:31
https://trac.sagemath.org/ticket/8327#comment:31
<ul>
<li><strong>work_issues</strong>
changed from <em>Find dependency. Needs more doctests and Sphinxified docstrings.</em> to <em>Find dependency or rebase first patch. Needs more doctests and Sphinxified docstrings.</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:30" title="Comment 30">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Gosh. And my patch fails to apply as well, in both hunks.
</p>
</blockquote>
<p>
O dear, where is my mind? I had accidentally reverted the order of the patches when I tried to apply my patch.
</p>
<p>
So, for the record: The first patch does need to be rebased (or a dependency must be given). The second patch applies cleanly.
</p>
TicketSimonKingTue, 15 Nov 2011 21:47:40 GMTwork_issues changed
https://trac.sagemath.org/ticket/8327#comment:32
https://trac.sagemath.org/ticket/8327#comment:32
<ul>
<li><strong>work_issues</strong>
changed from <em>Find dependency or rebase first patch. Needs more doctests and Sphinxified docstrings.</em> to <em>Find dependency or rebase first patch. Fix doctests. Needs more doctests and Sphinxified docstrings.</em>
</li>
</ul>
<p>
There are several doctest errors, even when one just has
</p>
<pre class="wiki">11761-cython-0.15-rebased.patch
trac12029_clonable_int_array_2_list.patch
trac_8327_universal_cyclotomic_field-cs.patch
trac_8327_universal_cyclotomic_field_new_cython-sk.patch
</pre><p>
with the obvious modification on <code>trac_8327_universal_cyclotomic_field-cs.patch</code>.
</p>
<p>
Namely:
</p>
<pre class="wiki"> sage -t devel/sage-main/sage/matrix/matrix2.pyx # 25 doctests failed
sage -t devel/sage-main/sage/modules/free_module.py # 2 doctests failed
sage -t devel/sage-main/sage/modular/modsym/p1list_nf.py # 5 doctests failed
sage -t devel/sage-main/sage/tests/startup.py # 1 doctests failed
sage -t devel/sage-main/sage/rings/universal_cyclotomic_field/universal_cyclotomic_field_c.pyx # 1 doctests failed
</pre><p>
This one here
</p>
<pre class="wiki">File "/mnt/local/king/SAGE/rebase/sage-4.8.alpha0/devel/sage-main/sage/rings/universal_cyclotomic_field/universal_cyclotomic_field_c.pyx", line 239:
sage: ZumbroichDecomposition_list( 6, 1 )
Expected:
array([1, 1])
Got:
[1, 1]
</pre><p>
could be caused by my patch, which touches <code>ZumbroichDecomposition</code>. However, I have no idea about the other errors.
</p>
Ticketstumpc5Wed, 16 Nov 2011 07:01:21 GMT
https://trac.sagemath.org/ticket/8327#comment:33
https://trac.sagemath.org/ticket/8327#comment:33
<p>
Thanks for working in this patch -- I will have a look at it this morning and see what the issues are...
</p>
<p>
Best, Christian
</p>
Ticketstumpc5Wed, 16 Nov 2011 07:11:21 GMT
https://trac.sagemath.org/ticket/8327#comment:34
https://trac.sagemath.org/ticket/8327#comment:34
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:33" title="Comment 33">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
Thanks for working in this patch -- I will have a look at it this morning and see what the issues are...
</p>
</blockquote>
<p>
I guess, I should first compile 4.7.2; that will take a while first...
</p>
TicketnthieryWed, 16 Nov 2011 07:39:38 GMT
https://trac.sagemath.org/ticket/8327#comment:35
https://trac.sagemath.org/ticket/8327#comment:35
<blockquote class="citation">
<blockquote class="citation">
<p>
Thanks for working in this patch -- I will have a look at it this morning and see what the issues are...
</p>
</blockquote>
</blockquote>
<p>
Cool!
</p>
<blockquote class="citation">
<p>
I guess, I should first compile 4.7.2; that will take a while first...
</p>
</blockquote>
<p>
4_8 alpha0 actually
</p>
<p>
Cheers,
</p>
Ticketstumpc5Wed, 16 Nov 2011 08:22:06 GMTstatus, description changed
https://trac.sagemath.org/ticket/8327#comment:36
https://trac.sagemath.org/ticket/8327#comment:36
<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/8327?action=diff&version=36">diff</a>)
</li>
</ul>
<p>
I found the bug and fixed it, all the above doctests now work (but only on 4.7.1, the other version is still building and will take more time).
</p>
<p>
What about "needs more doctests and Sphinxified docstrings"? I agree that the cython part is still not well documented. Could you be specific where I should explain better what's going on? Part of the problem is that computing the Zumbroich basis is something that is not simple to understand without really looking at the paper. And even after understanding that, everything becomes unreadable after implementing it in a (hopefully fairly) fast way in cython...
</p>
TicketSimonKingWed, 16 Nov 2011 09:19:02 GMT
https://trac.sagemath.org/ticket/8327#comment:37
https://trac.sagemath.org/ticket/8327#comment:37
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:36" title="Comment 36">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
I found the bug and fixed it, all the above doctests now work (but only on 4.7.1, the other version is still building and will take more time).
</p>
</blockquote>
<p>
Good! I'll test it a bit later.
</p>
<blockquote class="citation">
<p>
What about "needs more doctests and Sphinxified docstrings"? I agree that the cython part is still not well documented. Could you be specific where I should explain better what's going on?
</p>
</blockquote>
<p>
If by "you" you mean me, then I can't: The statement "needs more doctests ..." has been there before and is due to David Loeffler. I didn't come that far in reading the patch, I was first testing whether it applies, builds and passes tests.
</p>
TicketdavidloefflerWed, 16 Nov 2011 09:46:56 GMTwork_issues changed
https://trac.sagemath.org/ticket/8327#comment:38
https://trac.sagemath.org/ticket/8327#comment:38
<ul>
<li><strong>work_issues</strong>
changed from <em>Find dependency or rebase first patch. Fix doctests. Needs more doctests and Sphinxified docstrings.</em> to <em>Find dependency or rebase first patch. Fix doctests.</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:37" title="Comment 37">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
If by "you" you mean me, then I can't: The statement "needs more doctests ..." has been there before and is due to David Loeffler. I didn't come that far in reading the patch, I was first testing whether it applies, builds and passes tests.
</p>
</blockquote>
<p>
(That was 10 months ago and applied to a much earlier version of the patch.)
</p>
Ticketstumpc5Wed, 16 Nov 2011 09:50:38 GMT
https://trac.sagemath.org/ticket/8327#comment:39
https://trac.sagemath.org/ticket/8327#comment:39
<blockquote class="citation">
<p>
(That was 10 months ago and applied to a much earlier version of the patch.)
</p>
</blockquote>
<p>
I now remember seeing it back then, the patch indeed changed substantially since then. So I guess it simply needs a new review.
</p>
TicketdavidloefflerWed, 16 Nov 2011 09:53:51 GMT
https://trac.sagemath.org/ticket/8327#comment:40
https://trac.sagemath.org/ticket/8327#comment:40
<p>
It's perfectly permissible, and indeed a good idea, to clear the "work issues" field when uploading a new patch.
</p>
TicketSimonKingWed, 16 Nov 2011 10:42:24 GMTstatus, work_issues changed; dependencies set
https://trac.sagemath.org/ticket/8327#comment:41
https://trac.sagemath.org/ticket/8327#comment:41
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>dependencies</strong>
set to <em>#10771</em>
</li>
<li><strong>work_issues</strong>
changed from <em>Find dependency or rebase first patch. Fix doctests.</em> to <em>Rebase wrt #10771. Fix doctests.</em>
</li>
</ul>
<p>
When starting with sage-4.8.alpha0 plus the stuff from <a class="closed ticket" href="https://trac.sagemath.org/ticket/11761" title="enhancement: Upgrade Cython to 0.15.1 (closed: fixed)">#11761</a> (new cython), one quite big hunk of your new patch fails to apply. It concerns sage/categories/fields.py.
</p>
<p>
I found at least one reason for the mismatch: By trac ticket <a class="closed ticket" href="https://trac.sagemath.org/ticket/10771" title="enhancement: gcd and lcm for fraction fields (closed: fixed)">#10771</a> (sorry, it's one of mine...), <code>Fields()</code> has some <code>ElementMethods</code> (your patch assumes that there are none). Since <a class="closed ticket" href="https://trac.sagemath.org/ticket/10771" title="enhancement: gcd and lcm for fraction fields (closed: fixed)">#10771</a> has been merged in sage-4.7.2, I make this a dependency.
</p>
<p>
Well, I understand that you are building 4.7.2 anyway, aren't you?
</p>
Ticketstumpc5Wed, 16 Nov 2011 11:34:35 GMT
https://trac.sagemath.org/ticket/8327#comment:42
https://trac.sagemath.org/ticket/8327#comment:42
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:41" title="Comment 41">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
When starting with sage-4.8.alpha0 plus the stuff from <a class="closed ticket" href="https://trac.sagemath.org/ticket/11761" title="enhancement: Upgrade Cython to 0.15.1 (closed: fixed)">#11761</a> (new cython), one quite big hunk of your new patch fails to apply. It concerns sage/categories/fields.py.
</p>
<p>
I found at least one reason for the mismatch: By trac ticket <a class="closed ticket" href="https://trac.sagemath.org/ticket/10771" title="enhancement: gcd and lcm for fraction fields (closed: fixed)">#10771</a> (sorry, it's one of mine...), <code>Fields()</code> has some <code>ElementMethods</code> (your patch assumes that there are none). Since <a class="closed ticket" href="https://trac.sagemath.org/ticket/10771" title="enhancement: gcd and lcm for fraction fields (closed: fixed)">#10771</a> has been merged in sage-4.7.2, I make this a dependency.
</p>
<p>
Well, I understand that you are building 4.7.2 anyway, aren't you
</p>
</blockquote>
<p>
I am building 4.7.2 and 4.8, but te later just failed to build... l will do the rebase after 4.7.2 has finished (or you do it, if
you want to, it will basically take the whole day until the building is done)
</p>
TicketSimonKingWed, 16 Nov 2011 11:44:14 GMT
https://trac.sagemath.org/ticket/8327#comment:43
https://trac.sagemath.org/ticket/8327#comment:43
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:42" title="Comment 42">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
I am building 4.7.2 and 4.8, but te later just failed to build...
</p>
</blockquote>
<p>
What, 4.8.alpha0 (or alpha1) failed to build?? That should not happen. Perhaps you can report on sage-release?
</p>
TicketnthieryWed, 16 Nov 2011 12:51:59 GMT
https://trac.sagemath.org/ticket/8327#comment:44
https://trac.sagemath.org/ticket/8327#comment:44
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:41" title="Comment 41">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
When starting with sage-4.8.alpha0 plus the stuff from <a class="closed ticket" href="https://trac.sagemath.org/ticket/11761" title="enhancement: Upgrade Cython to 0.15.1 (closed: fixed)">#11761</a> (new cython), one quite big hunk of your new patch fails to apply. It concerns sage/categories/fields.py.
</p>
<p>
I found at least one reason for the mismatch: By trac ticket <a class="closed ticket" href="https://trac.sagemath.org/ticket/10771" title="enhancement: gcd and lcm for fraction fields (closed: fixed)">#10771</a> (sorry, it's one of mine...), <code>Fields()</code> has some <code>ElementMethods</code> (your patch assumes that there are none). Since <a class="closed ticket" href="https://trac.sagemath.org/ticket/10771" title="enhancement: gcd and lcm for fraction fields (closed: fixed)">#10771</a> has been merged in sage-4.7.2, I make this a dependency.
</p>
</blockquote>
<p>
IIRC, this patch already applies on 4.7.2: I already rebased it last week upon 10771. Ah ah, I was in a hurry, and probably forgot to upload it on trac, sorry. Please take the latest version from the sage-combinat patch server.
</p>
Ticketstumpc5Wed, 16 Nov 2011 13:27:48 GMT
https://trac.sagemath.org/ticket/8327#comment:45
https://trac.sagemath.org/ticket/8327#comment:45
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:44" title="Comment 44">nthiery</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:41" title="Comment 41">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
When starting with sage-4.8.alpha0 plus the stuff from <a class="closed ticket" href="https://trac.sagemath.org/ticket/11761" title="enhancement: Upgrade Cython to 0.15.1 (closed: fixed)">#11761</a> (new cython), one quite big hunk of your new patch fails to apply. It concerns sage/categories/fields.py.
</p>
<p>
I found at least one reason for the mismatch: By trac ticket <a class="closed ticket" href="https://trac.sagemath.org/ticket/10771" title="enhancement: gcd and lcm for fraction fields (closed: fixed)">#10771</a> (sorry, it's one of mine...), <code>Fields()</code> has some <code>ElementMethods</code> (your patch assumes that there are none). Since <a class="closed ticket" href="https://trac.sagemath.org/ticket/10771" title="enhancement: gcd and lcm for fraction fields (closed: fixed)">#10771</a> has been merged in sage-4.7.2, I make this a dependency.
</p>
</blockquote>
<p>
IIRC, this patch already applies on 4.7.2: I already rebased it last week upon 10771. Ah ah, I was in a hurry, and probably forgot to upload it on trac, sorry. Please take the latest version from the sage-combinat patch server.
</p>
</blockquote>
<p>
I did the same mistake the other way round: I did changes about 5 weeks ago, and only putted it on trac but not on the combinat queue...
</p>
<p>
I just merged both versions, and the doctest should pass now, and it should be rebased (both only if I didn't do any mistakes...).
</p>
TicketSimonKingWed, 16 Nov 2011 14:41:53 GMT
https://trac.sagemath.org/ticket/8327#comment:46
https://trac.sagemath.org/ticket/8327#comment:46
<p>
The new patch does not apply, because of the hunk
</p>
<pre class="wiki">--- all.py
+++ all.py
@@ -1,3 +1,5 @@
+from sage.misc.lazy_import import lazy_import
+
from category import is_Category, Category, HomCategory
</pre><p>
The point is that this hunk shold be totally removed, because lazy import is not needed in sage/categories/all.py.
</p>
<p>
For testing, I'm doing this change myself. But please post a patch without that hunk
</p>
TicketSimonKingWed, 16 Nov 2011 14:53:30 GMTstatus changed
https://trac.sagemath.org/ticket/8327#comment:47
https://trac.sagemath.org/ticket/8327#comment:47
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
Sorry, lazy_import <em>is</em> used. So, don't delete the patch. However, it does not apply. Reason: The first line of sage/categories/all.py currently is
</p>
<pre class="wiki">from category import is_Category, Category, HomCategory, AbstractCategory
</pre><p>
My guess is that you worked on top of Nicolas's "multiple realizations" patch.
</p>
<p>
So, either submit a patch on trac that differs from the one in the combinat queue, or name <a class="closed ticket" href="https://trac.sagemath.org/ticket/7980" title="enhancement: Implement generic support for parents with (multiple) realizations (closed: fixed)">#7980</a> as a dependency.
</p>
<p>
Suggestion: I can manually rebase your patch, so that I can test (and review) it now. The merge conflict is trivial and can be sorted out later.
</p>
Ticketstumpc5Wed, 16 Nov 2011 14:57:03 GMT
https://trac.sagemath.org/ticket/8327#comment:48
https://trac.sagemath.org/ticket/8327#comment:48
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:47" title="Comment 47">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Suggestion: I can manually rebase your patch, so that I can test (and review) it now. The merge conflict is trivial and can be sorted out later.
</p>
</blockquote>
<p>
I am a little lost now: you don't want the current patch but the one I
had before without Nicolas' rebase?
</p>
TicketSimonKingWed, 16 Nov 2011 15:33:54 GMT
https://trac.sagemath.org/ticket/8327#comment:49
https://trac.sagemath.org/ticket/8327#comment:49
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:48" title="Comment 48">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
I am a little lost now: you don't want the current patch but the one I
had before without Nicolas' rebase?
</p>
</blockquote>
<p>
The patch that is attached here does not apply on pure sage-4.7.2 or sage-4.8.alpha0. The reason is that this patch depends on Nicolas's patch from <a class="closed ticket" href="https://trac.sagemath.org/ticket/7980" title="enhancement: Implement generic support for parents with (multiple) realizations (closed: fixed)">#7980</a>.
</p>
<p>
So, at some point, the merge conflict has to be sorted out. However, for my review I don't need that you rebase it now.
</p>
TicketSimonKingWed, 16 Nov 2011 16:01:07 GMT
https://trac.sagemath.org/ticket/8327#comment:50
https://trac.sagemath.org/ticket/8327#comment:50
<p>
I get one error in startup.py:
</p>
<pre class="wiki">File "/mnt/local/king/SAGE/rebase/sage-4.8.alpha0/devel/sage-main/sage/tests/startup.py", line 13:
sage: sage0("'numpy' in sys.modules")
Expected:
False
Got:
True
</pre><p>
Do you import numpy at startup? Nicolas has just found that ticket <a class="closed ticket" href="https://trac.sagemath.org/ticket/11714" title="enhancement: ensure that numpy is not imported on startup (closed: fixed)">#11714</a> explicitly avoids numpy being loaded at startup.
</p>
TicketSimonKingWed, 16 Nov 2011 16:04:40 GMT
https://trac.sagemath.org/ticket/8327#comment:51
https://trac.sagemath.org/ticket/8327#comment:51
<p>
You import and cimport numpy. Nicolas suggest to try and lazy_import numpy instead of import (cimport should be fine).
</p>
TicketSimonKingWed, 16 Nov 2011 16:16:41 GMT
https://trac.sagemath.org/ticket/8327#comment:52
https://trac.sagemath.org/ticket/8327#comment:52
<p>
... or lazy-import the stuff from sage/rings/universal_cyclotomic_field/all.py? I'm preparing a patch that does this.
</p>
TicketSimonKingWed, 16 Nov 2011 16:21:44 GMTdescription changed
https://trac.sagemath.org/ticket/8327#comment:53
https://trac.sagemath.org/ticket/8327#comment:53
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/8327?action=diff&version=53">diff</a>)
</li>
</ul>
<p>
OK, done.
</p>
<p>
When applying both patches, one gets
</p>
<pre class="wiki">sage: 'numpy' in sys.modules
False
sage: sage0("'numpy' in sys.modules")
False
sage: UCF
Universal Cyclotomic Field endowed with the Zumbroich basis
sage: 'numpy' in sys.modules
True
sage: sage0("'numpy' in sys.modules")
False
</pre><p>
So, numpy is only imported when needed, and this is what <a class="closed ticket" href="https://trac.sagemath.org/ticket/11714" title="enhancement: ensure that numpy is not imported on startup (closed: fixed)">#11714</a> was about.
</p>
<p>
Apply trac_8327_universal_cyclotomic_field-cs.patch trac8327_lazy_import_UCF.patch
</p>
TicketSimonKingWed, 16 Nov 2011 22:02:11 GMTstatus, work_issues changed
https://trac.sagemath.org/ticket/8327#comment:54
https://trac.sagemath.org/ticket/8327#comment:54
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>work_issues</strong>
changed from <em>Rebase wrt #10771. Fix doctests.</em> to <em>Rebase wrt #10771. Fix one doctest.</em>
</li>
</ul>
<p>
Starting with sage-4.8.alpha0 plus the new Cython plus your patch (rebased in the obvious way) plus the lazy_import patch, we are down to one error:
</p>
<pre class="wiki">sage -t "devel/sage-main/sage/rings/universal_cyclotomic_field/universal_cyclotomic_field.py"
**********************************************************************
File "/mnt/local/king/SAGE/rebase/sage-4.8.alpha0/devel/sage-main/sage/rings/universal_cyclotomic_field/universal_cyclotomic_field.py", line 274:
sage: UCF.is_subring(UCF)
Expected:
True
Got:
False
**********************************************************************
1 items had failures:
1 of 4 in __main__.example_5
***Test Failed*** 1 failures.
For whitespace errors, see the file /mnt/local/king/.sage/tmp/universal_cyclotomic_field_4421.py
[3.3 s]
</pre><p>
Any idea why that occurs?
</p>
Ticketstumpc5Thu, 17 Nov 2011 08:34:36 GMT
https://trac.sagemath.org/ticket/8327#comment:55
https://trac.sagemath.org/ticket/8327#comment:55
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:54" title="Comment 54">SimonKing</a>:
</p>
<blockquote class="citation">
<blockquote>
<p>
sage: UCF.is_subring(UCF)
</p>
</blockquote>
<p>
Expected:
</p>
<blockquote>
<p>
True
</p>
</blockquote>
<p>
Got:
</p>
<blockquote>
<p>
False
</p>
</blockquote>
</blockquote>
<p>
UCF.is_subring(UCF) just returns
</p>
<pre class="wiki">sage: UCF is UCF
</pre><p>
and as <code>UniversalCyclotomicField</code> inherits from <code>UniqueRepresentation</code>, this should return <code>True</code>. I don't know if this error might come from some lazy import which makes UCF not unique, or if something changed in <code>UniqueRepresentation</code>? Nicolas might know...
</p>
TicketnthieryThu, 17 Nov 2011 13:42:49 GMT
https://trac.sagemath.org/ticket/8327#comment:56
https://trac.sagemath.org/ticket/8327#comment:56
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:55" title="Comment 55">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
UCF.is_subring(UCF) just returns
</p>
<pre class="wiki">sage: UCF is UCF
</pre><p>
and as <code>UniversalCyclotomicField</code> inherits from <code>UniqueRepresentation</code>, this should return <code>True</code>.
</p>
</blockquote>
<p>
That's independent of <a class="missing wiki">UniqueRepresentation?</a>: <code></code>X is X<code></code> is true for any
Python object X.
</p>
<p>
So the test above breaks, my best bet is that the is_ring method that
gets called is the wrong one.
</p>
<p>
Good chase!
</p>
<blockquote>
<p>
Nicolas
</p>
</blockquote>
Ticketstumpc5Fri, 18 Nov 2011 11:21:30 GMT
https://trac.sagemath.org/ticket/8327#comment:57
https://trac.sagemath.org/ticket/8327#comment:57
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:56" title="Comment 56">nthiery</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:55" title="Comment 55">stumpc5</a>:
So the test above breaks, my best bet is that the is_ring method that
gets called is the wrong one.
</p>
</blockquote>
<p>
I found the problem with building sage (see <a class="closed ticket" href="https://trac.sagemath.org/ticket/11970" title="defect: r-2.10.1.p4 may fail to configure with readline (BLOCKS Sage from ... (closed: fixed)">#11970</a>).
</p>
<p>
4.7.2 has finished, and I now build 4.8.alpha1. I will investigate the above problem as soon as it is finished...
</p>
Ticketstumpc5Fri, 18 Nov 2011 17:15:15 GMTstatus, description changed
https://trac.sagemath.org/ticket/8327#comment:58
https://trac.sagemath.org/ticket/8327#comment:58
<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/8327?action=diff&version=58">diff</a>)
</li>
</ul>
<p>
It is rebased and applies now on a new 4.8.alpha1 (NOT alpha0, but alpha1!)
</p>
<p>
There was a syntax error on the plain 4.8.alpha1, which I first had to get rid off.
</p>
<p>
Concerning the is_subring method, I get the following behaviour:
</p>
<pre class="wiki">sage: UCF is UCF
True
sage: UCF.is_subring??
def is_subring(self,other):
r"""
Returns currently True if ``self`` and ``other`` coincide.
EXAMPLES::
sage: UCF.is_subring(UCF)
True
"""
return other is self
sage: UCF.is_subring(UCF)
False
</pre><p>
This behaviour seems to be wired! But when replacing the lazy import of the UCF by a proper import in sage.rings.all, the wired behaviour disappears, so the problem must be something with the lazy import!
</p>
TicketnthieryFri, 18 Nov 2011 22:42:58 GMT
https://trac.sagemath.org/ticket/8327#comment:59
https://trac.sagemath.org/ticket/8327#comment:59
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:58" title="Comment 58">stumpc5</a>:
</p>
<blockquote class="citation">
<pre class="wiki">sage: UCF.is_subring(UCF)
False
</pre><p>
This behaviour seems to be wired! But when replacing the lazy import of the UCF by a proper import in sage.rings.all, the wired behaviour disappears, so the problem must be something with the lazy import!
</p>
</blockquote>
<p>
Ach ja! Sorry, I should have thought about this: it is an instance of
<a class="closed ticket" href="https://trac.sagemath.org/ticket/10906" title="defect: lazy import can break unique representation (closed: duplicate)">#10906</a>: "lazy import can break unique representation". So UCF should
not be lazy imported. One probably can't lazy cimport numpy in
universal_cyclotomic_field_c.pyx. Hopefully one could replace, in
universal_cyclotomic_field.py:
</p>
<pre class="wiki">from sage.rings.universal_cyclotomic_field.universal_cyclotomic_field_c import *
</pre><p>
by explicit lazy imports of the various functions:
</p>
<pre class="wiki">lazy_import('sage.rings.universal_cyclotomic_field.universal_cyclotomic_field_c', 'ZumbroichBasis...')
</pre><p>
or maybe just lazy import that module as, say, ucfc, and qualify all
its functions with it.
</p>
<p>
I don't have a working Sage in which I could insert the UCF patch
right now. But let me know if you need help!
</p>
<p>
Cheers,
</p>
<blockquote>
<p>
Nicolas
</p>
</blockquote>
Ticketstumpc5Fri, 18 Nov 2011 23:45:58 GMT
https://trac.sagemath.org/ticket/8327#comment:60
https://trac.sagemath.org/ticket/8327#comment:60
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:59" title="Comment 59">nthiery</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:58" title="Comment 58">stumpc5</a>:
</p>
<pre class="wiki">lazy_import('sage.rings.universal_cyclotomic_field.universal_cyclotomic_field_c', 'ZumbroichBasis...')
</pre></blockquote>
<p>
I now do that, and all tests pass -- but I somehow missed why we want to lazy import anything; is it about importing numpy?
</p>
TicketSimonKingFri, 18 Nov 2011 23:57:18 GMT
https://trac.sagemath.org/ticket/8327#comment:61
https://trac.sagemath.org/ticket/8327#comment:61
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:60" title="Comment 60">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
I now do that, and all tests pass -- but I somehow missed why we want to lazy import anything; is it about importing numpy?
</p>
</blockquote>
<p>
Apparently, there was a patch recently merged that explicitly had the purpose of "do not import numpy at startup". SO, I guess it would be better to not re-introduce the numpy import, unless we <em>really</em> need it.
</p>
Ticketstumpc5Sat, 19 Nov 2011 08:29:35 GMT
https://trac.sagemath.org/ticket/8327#comment:62
https://trac.sagemath.org/ticket/8327#comment:62
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:61" title="Comment 61">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:60" title="Comment 60">stumpc5</a>:
Apparently, there was a patch recently merged that explicitly had the purpose of "do not import numpy at startup". SO, I guess it would be better to not re-introduce the numpy import, unless we <em>really</em> need it.
</p>
</blockquote>
<p>
I see -- I made some mistake last night, but now, the cython part is lazy imported, and all tests pass...
</p>
TicketnthierySat, 19 Nov 2011 09:11:44 GMT
https://trac.sagemath.org/ticket/8327#comment:63
https://trac.sagemath.org/ticket/8327#comment:63
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:62" title="Comment 62">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
I see -- I made some mistake last night, but now, the cython part is lazy imported, and all tests pass...
</p>
</blockquote>
<p>
Cool. Thanks! This is as much saved for Sage's startup.
</p>
<p>
Cheers,
</p>
<blockquote>
<p>
Nicolas
</p>
</blockquote>
TicketmhansenSun, 18 Dec 2011 13:19:40 GMT
https://trac.sagemath.org/ticket/8327#comment:64
https://trac.sagemath.org/ticket/8327#comment:64
<p>
Ping. I think we should try to get this in shortly.
</p>
Ticketstumpc5Sun, 18 Dec 2011 13:24:42 GMT
https://trac.sagemath.org/ticket/8327#comment:65
https://trac.sagemath.org/ticket/8327#comment:65
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:64" title="Comment 64">mhansen</a>:
</p>
<blockquote class="citation">
<p>
Ping. I think we should try to get this in shortly.
</p>
</blockquote>
<p>
+1.
</p>
<p>
Simon, are you planning to review it? Or anyone else volunteering?
</p>
<p>
Christian
</p>
TicketSimonKingSun, 18 Dec 2011 14:13:57 GMT
https://trac.sagemath.org/ticket/8327#comment:66
https://trac.sagemath.org/ticket/8327#comment:66
<p>
I'm a bit lost. I am listed as an author, and some comments mention a patch of mine. But where is that patch? I'm talking about trac_8327_universal_cyclotomic_field_new_cython-sk.patch
</p>
<p>
Similarly, where is trac8327_lazy_import_UCF.patch ?
</p>
<p>
I thought it is impossible (unless for administrators) to remove an attachment from trac?
</p>
<p>
Also, there are two work issues mentioned. Are these fixed? I.e., is it really "needs review", or is it "needs work"?
</p>
Ticketstumpc5Sun, 18 Dec 2011 14:26:31 GMTwork_issues deleted
https://trac.sagemath.org/ticket/8327#comment:67
https://trac.sagemath.org/ticket/8327#comment:67
<ul>
<li><strong>work_issues</strong>
<em>Rebase wrt #10771. Fix one doctest.</em> deleted
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:66" title="Comment 66">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
I'm a bit lost. I am listed as an author, and some comments mention a patch of mine. But where is that patch? I'm talking about trac_8327_universal_cyclotomic_field_new_cython-sk.patch
</p>
<p>
Similarly, where is trac8327_lazy_import_UCF.patch ?
</p>
</blockquote>
<p>
As your patches were only a few lines, I folded them into mine so the whole ticket is easier to handle (and then deleted your patches to not confuse the patchbot).
</p>
<blockquote class="citation">
<p>
I thought it is impossible (unless for administrators) to remove an attachment from trac?
</p>
</blockquote>
<p>
I guess, I have admin rights then.
</p>
<blockquote class="citation">
<p>
Also, there are two work issues mentioned. Are these fixed? I.e., is it really "needs review", or is it "needs work"?
</p>
</blockquote>
<p>
sorry for not deleting them, I fixed both 4 weeks ago (though I don't know if there is another rebase needed for
sage-4.8.alpha4).
</p>
<p>
Best, Christian
</p>
TicketdavidloefflerSun, 18 Dec 2011 14:38:06 GMT
https://trac.sagemath.org/ticket/8327#comment:68
https://trac.sagemath.org/ticket/8327#comment:68
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:67" title="Comment 67">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
(though I don't know if there is another rebase needed for sage-4.8.alpha4).
</p>
</blockquote>
<p>
No need -- patch applies fine to 4.8.alpha4. The new files pass their doctests (I haven't run a full doctest cycle).
</p>
TicketSimonKingSun, 18 Dec 2011 14:42:50 GMT
https://trac.sagemath.org/ticket/8327#comment:69
https://trac.sagemath.org/ticket/8327#comment:69
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:67" title="Comment 67">stumpc5</a>:
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
I thought it is impossible (unless for administrators) to remove an attachment from trac?
</p>
</blockquote>
<p>
I guess, I have admin rights then.
</p>
</blockquote>
<p>
If you just didn't want to confuse the patchbot, you should insert a line in some comment, stating what patches have to be applied in what order:
</p>
<p>
Apply trac_8327_universal_cyclotomic_field-cs.patch
</p>
<p>
But then, what is the status of your "syntax error" patch?
</p>
Ticketstumpc5Sun, 18 Dec 2011 14:54:17 GMT
https://trac.sagemath.org/ticket/8327#comment:70
https://trac.sagemath.org/ticket/8327#comment:70
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:69" title="Comment 69">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
But then, what is the status of your "syntax error" patch?
</p>
</blockquote>
<p>
I wasn't able to run 4.8.alpha1 due to a non-ascii character in /rings/arith.py. The patch only deletes this character -- I don't know if it is still present in 4.8.alpha4, but I will check after it has finished building.
</p>
TicketdavidloefflerSat, 10 Mar 2012 14:41:25 GMTdependencies, description changed
https://trac.sagemath.org/ticket/8327#comment:71
https://trac.sagemath.org/ticket/8327#comment:71
<ul>
<li><strong>dependencies</strong>
changed from <em>#10771</em> to <em>#4539 #10771</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/8327?action=diff&version=71">diff</a>)
</li>
</ul>
<p>
Apply trac_8327-rebased_for_v5.patch
</p>
<p>
(for the patchbot).
</p>
<p>
The previous patch conflicts with <a class="closed ticket" href="https://trac.sagemath.org/ticket/4539" title="enhancement: plural wrapper (closed: fixed)">#4539</a>, so I have done a new rebased patch. There was one change in <code>free_module.py</code> which was rejected, and whose purpose was not at all clear to me, so I didn't include it in the rebased patch -- all doctests seem to pass without it.
</p>
TicketdavidloefflerThu, 29 Mar 2012 12:16:45 GMTstatus changed
https://trac.sagemath.org/ticket/8327#comment:72
https://trac.sagemath.org/ticket/8327#comment:72
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
And now it's failing to apply again with the latest beta (5.0.beta11), due to a conflict in sage/categories/all.py coming from with <a class="closed ticket" href="https://trac.sagemath.org/ticket/7980" title="enhancement: Implement generic support for parents with (multiple) realizations (closed: fixed)">#7980</a>.
</p>
TicketdavidloefflerFri, 30 Mar 2012 07:57:20 GMTstatus changed
https://trac.sagemath.org/ticket/8327#comment:73
https://trac.sagemath.org/ticket/8327#comment:73
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
I've re-rebased the patch.
</p>
TicketdavidloefflerFri, 30 Mar 2012 08:44:36 GMT
https://trac.sagemath.org/ticket/8327#comment:74
https://trac.sagemath.org/ticket/8327#comment:74
<p>
Apply trac_8327-rebased_for_v5.patch, trac_8327-docfix.patch
</p>
<p>
(I got some error messages building the documentation -- the above patch fixes them.)
</p>
TicketdavidloefflerFri, 30 Mar 2012 08:53:34 GMTdescription, author changed
https://trac.sagemath.org/ticket/8327#comment:75
https://trac.sagemath.org/ticket/8327#comment:75
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/8327?action=diff&version=75">diff</a>)
</li>
<li><strong>author</strong>
changed from <em>Christian Stump, Simon King</em> to <em>Christian Stump, Simon King, David Loeffler</em>
</li>
</ul>
TicketdavidloefflerFri, 30 Mar 2012 08:54:18 GMTdependencies changed
https://trac.sagemath.org/ticket/8327#comment:76
https://trac.sagemath.org/ticket/8327#comment:76
<ul>
<li><strong>dependencies</strong>
changed from <em>#4539 #10771</em> to <em>#4539 #10771 #7980</em>
</li>
</ul>
TicketdavidloefflerFri, 30 Mar 2012 08:55:03 GMT
https://trac.sagemath.org/ticket/8327#comment:77
https://trac.sagemath.org/ticket/8327#comment:77
<p>
Apply trac_8327-rebased_for_v5.patch, trac_8327-docfix.patch
</p>
<p>
(patchbot's confused again)
</p>
TicketnthieryFri, 30 Mar 2012 09:04:14 GMT
https://trac.sagemath.org/ticket/8327#comment:78
https://trac.sagemath.org/ticket/8327#comment:78
<blockquote>
<p>
Hi guys,
</p>
</blockquote>
<p>
Just my 2 cents, but Christian did an awfull amount of work on this patch; I really appreciate your efforts to help him get this in (I should participate to that too!), and I know you added yourself to share the blame in case of trouble later; still I feel he deserves to be single author. What do you think?
</p>
<p>
Cheers,
</p>
<blockquote>
<p>
Nicolas
</p>
</blockquote>
TicketdavidloefflerFri, 30 Mar 2012 09:06:58 GMTauthor changed; reviewer set
https://trac.sagemath.org/ticket/8327#comment:79
https://trac.sagemath.org/ticket/8327#comment:79
<ul>
<li><strong>reviewer</strong>
set to <em>David Loeffler</em>
</li>
<li><strong>author</strong>
changed from <em>Christian Stump, Simon King, David Loeffler</em> to <em>Christian Stump, Simon King</em>
</li>
</ul>
<p>
Sure -- my contribution was just rebasing + reformatting some docstrings. I've removed myself as author (but added myself as reviewer).
</p>
Ticketstumpc5Fri, 30 Mar 2012 10:25:12 GMT
https://trac.sagemath.org/ticket/8327#comment:80
https://trac.sagemath.org/ticket/8327#comment:80
<p>
Hi,
</p>
<p>
Thanks for pushing the patch forward - I actually don't care whose name appears in the authors field, but I would appreciate if we can get the patch ready soon (maybe even for 5.0)!
</p>
<p>
I have a slightly newer version of the patch locally containing some missing docstrings and stuff. Do you mind if I delete all 4 attached patches and push one patch containing
</p>
<p>
+ all my new docstrings
</p>
<p>
+ rebased for 5.0.beta6 (is that recent enough? I haven't build anything newer, so I cannot rebase the patch for beta11; I could do that tomorrow after building.)
</p>
<p>
+ David's improvements in the docstrings
</p>
<p>
?
</p>
<p>
Best, Christian
</p>
TicketdavidloefflerFri, 30 Mar 2012 10:34:19 GMT
https://trac.sagemath.org/ticket/8327#comment:81
https://trac.sagemath.org/ticket/8327#comment:81
<p>
I think it would be rather counterproductive to erase a patch that applies to the current beta, and replace it with one that almost certainly wouldn't. The most recent bout of conflicts was caused by <a class="closed ticket" href="https://trac.sagemath.org/ticket/7980" title="enhancement: Implement generic support for parents with (multiple) realizations (closed: fixed)">#7980</a>, which was only merged in beta11. So please build a copy of beta11 before you do any further work on this.
</p>
Ticketstumpc5Fri, 30 Mar 2012 10:42:51 GMT
https://trac.sagemath.org/ticket/8327#comment:82
https://trac.sagemath.org/ticket/8327#comment:82
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:81" title="Comment 81">davidloeffler</a>:
</p>
<blockquote class="citation">
<p>
I think it would be rather counterproductive to erase a patch that applies to the current beta, and replace it with one that almost certainly wouldn't. The most recent bout of conflicts was caused by <a class="closed ticket" href="https://trac.sagemath.org/ticket/7980" title="enhancement: Implement generic support for parents with (multiple) realizations (closed: fixed)">#7980</a>, which was only merged in beta11. So please build a copy of beta11 before you do any further work on this.
</p>
</blockquote>
<p>
Alright, I will then first build and do attach another version tomorrow then.
</p>
TicketnthieryFri, 30 Mar 2012 16:02:34 GMT
https://trac.sagemath.org/ticket/8327#comment:83
https://trac.sagemath.org/ticket/8327#comment:83
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:82" title="Comment 82">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:81" title="Comment 81">davidloeffler</a>:
</p>
<blockquote class="citation">
<p>
I think it would be rather counterproductive to erase a patch that applies to the current beta, and replace it with one that almost certainly wouldn't. The most recent bout of conflicts was caused by <a class="closed ticket" href="https://trac.sagemath.org/ticket/7980" title="enhancement: Implement generic support for parents with (multiple) realizations (closed: fixed)">#7980</a>, which was only merged in beta11. So please build a copy of beta11 before you do any further work on this.
</p>
</blockquote>
<p>
Alright, I will then first build and do attach another version tomorrow then.
</p>
</blockquote>
<p>
For the record: if Christian's latest version is that on the sage-combinat patch server, then it should be already based on <a class="closed ticket" href="https://trac.sagemath.org/ticket/7980" title="enhancement: Implement generic support for parents with (multiple) realizations (closed: fixed)">#7980</a> and likely to apply smoothly.
</p>
Ticketstumpc5Fri, 30 Mar 2012 18:23:11 GMTdescription changed
https://trac.sagemath.org/ticket/8327#comment:84
https://trac.sagemath.org/ticket/8327#comment:84
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/8327?action=diff&version=84">diff</a>)
</li>
</ul>
Ticketstumpc5Fri, 30 Mar 2012 18:24:24 GMT
https://trac.sagemath.org/ticket/8327#comment:85
https://trac.sagemath.org/ticket/8327#comment:85
<p>
I attached a new version that contains everything I have, which is rebased for 5.0.beta11, and also contains David's docfixes.
</p>
TicketdavidloefflerFri, 30 Mar 2012 20:06:25 GMT
https://trac.sagemath.org/ticket/8327#comment:86
https://trac.sagemath.org/ticket/8327#comment:86
<p>
Apply trac_8327_universal_cyclotomic_field-cs.patch
</p>
<p>
(for patchbot)
</p>
TicketdavidloefflerFri, 30 Mar 2012 20:53:03 GMTstatus changed; reviewer deleted
https://trac.sagemath.org/ticket/8327#comment:87
https://trac.sagemath.org/ticket/8327#comment:87
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>reviewer</strong>
<em>David Loeffler</em> deleted
</li>
</ul>
<pre class="wiki">sage -t -force_lib devel/sage-8327/sage/rings/universal_cyclotomic_field/universal_cyclotomic_field_c.pyx
**********************************************************************
File "/storage/masiao/sage-5.0.beta11-patchbot/devel/sage-8327/sage/rings/universal_cyclotomic_field/universal_cyclotomic_field_c.pyx", line 239:
sage: ZumbroichDecomposition_list( 6, 1 )
Expected:
array([1, 1])
Got:
[1, 1]
**********************************************************************
1 items had failures:
1 of 7 in __main__.example_7
***Test Failed*** 1 failures.
For whitespace errors, see the file /home/masiao/.sage/tmp/fermat-21656/universal_cyclotomic_field_c_22797.py
[1.7 s]
sage -t -force_lib devel/sage-8327/sage/rings/polynomial/polynomial_quotient_ring.py
**********************************************************************
File "/storage/masiao/sage-5.0.beta11-patchbot/devel/sage-8327/sage/rings/polynomial/polynomial_quotient_ring.py", line 262:
sage: [s for s in dir(Q.category().element_class) if not s.startswith('_')]
Expected:
['cartesian_product', 'gcd', 'is_idempotent', 'is_one', 'lcm', 'lift']
Got:
['cartesian_product', 'gcd', 'is_idempotent', 'is_one', 'is_unit', 'lcm', 'lift']
**********************************************************************
1 items had failures:
1 of 30 in __main__.example_2
***Test Failed*** 1 failures.
For whitespace errors, see the file /home/masiao/.sage/tmp/fermat-21656/polynomial_quotient_ring_22987.py
[42.5 s]
sage -t -force_lib devel/sage-8327/sage/tests/startup.py
**********************************************************************
File "/storage/masiao/sage-5.0.beta11-patchbot/devel/sage-8327/sage/tests/startup.py", line 13:
sage: sage0("'numpy' in sys.modules")
Expected:
False
Got:
True
**********************************************************************
</pre><p>
(All three of these failures had already been reported, and fixed, before this latest patch re-broke them.)
</p>
Ticketstumpc5Sat, 31 Mar 2012 07:19:51 GMT
https://trac.sagemath.org/ticket/8327#comment:88
https://trac.sagemath.org/ticket/8327#comment:88
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:87" title="Comment 87">davidloeffler</a>:
</p>
<blockquote class="citation">
<p>
(All three of these failures had already been reported, and fixed, before this latest patch re-broke them.)
</p>
</blockquote>
<p>
These were not fixed in the docfix patch but in the rebase patch, so I didn't see them. (I actually fixed the first, but forgot to refresh the patch afterwards.)
</p>
<p>
I now attached the new version that should have the issues fixed. including the last which seems to me like not related to this patch, is it?
</p>
TicketdavidloefflerSat, 31 Mar 2012 08:20:04 GMTwork_issues set
https://trac.sagemath.org/ticket/8327#comment:89
https://trac.sagemath.org/ticket/8327#comment:89
<ul>
<li><strong>work_issues</strong>
set to <em>don't import numpy</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:88" title="Comment 88">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
I now attached the new version that should have the issues fixed. including the last which seems to me like not related to this patch, is it?
</p>
</blockquote>
<p>
It is related to your patch, as discussed in the comment thread above. It is important that Sage <strong>must not</strong> import numpy at startup, because it takes a very long time to load (cf. tickets <a class="closed ticket" href="https://trac.sagemath.org/ticket/11714" title="enhancement: ensure that numpy is not imported on startup (closed: fixed)">#11714</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/6494" title="defect: sage should *never* ever import numpy by default on startup. Yet ... (closed: invalid)">#6494</a>, and several others I think). Your patch causes numpy to be imported at startup; Nicolas and Simon put in a fair bit of work using <code>lazy_import</code> to stop this happening (see comments 50-53). You will see that <a class="missing attachment">trac_8327-rebased_for_v5.patch</a> does not need to disable this test in startup.py as you have done.
</p>
Ticketstumpc5Sat, 31 Mar 2012 08:56:25 GMT
https://trac.sagemath.org/ticket/8327#comment:90
https://trac.sagemath.org/ticket/8327#comment:90
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:89" title="Comment 89">davidloeffler</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:88" title="Comment 88">stumpc5</a>:
</p>
</blockquote>
<blockquote class="citation">
<p>
It is related to your patch, as discussed in the comment thread above. It is important that Sage <strong>must not</strong> import numpy at startup, because it takes a very long time to load (cf. tickets <a class="closed ticket" href="https://trac.sagemath.org/ticket/11714" title="enhancement: ensure that numpy is not imported on startup (closed: fixed)">#11714</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/6494" title="defect: sage should *never* ever import numpy by default on startup. Yet ... (closed: invalid)">#6494</a>, and several others I think). Your patch causes numpy to be imported at startup; Nicolas and Simon put in a fair bit of work using <code>lazy_import</code> to stop this happening (see comments 50-53). You will see that <a class="missing attachment">trac_8327-rebased_for_v5.patch</a> does not need to disable this test in startup.py as you have done.
</p>
</blockquote>
<p>
Were is my mind, thanks for telling me again! -- The patch attached here and the one I had on the combinat queue were both changed, I now made numpy got lazy imported again. I will post the patch as soon as there are no further test failures.
</p>
Ticketstumpc5Sat, 31 Mar 2012 09:19:18 GMTstatus changed; work_issues deleted
https://trac.sagemath.org/ticket/8327#comment:91
https://trac.sagemath.org/ticket/8327#comment:91
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>work_issues</strong>
<em>don't import numpy</em> deleted
</li>
</ul>
<p>
numpy is now lazy imported:
</p>
<pre class="wiki">sage: 'numpy' in sys.modules
False
sage: sage0("'numpy' in sys.modules")
False
sage: UCF
Universal Cyclotomic Field endowed with the Zumbroich basis
sage: 'numpy' in sys.modules
False
sage: E(5)
E(5)
sage: 'numpy' in sys.modules
True
sage: sage0("'numpy' in sys.modules")
False
</pre><p>
I hope I haven't forgotten any further things!
</p>
TicketdavidloefflerSat, 31 Mar 2012 11:00:01 GMT
https://trac.sagemath.org/ticket/8327#comment:92
https://trac.sagemath.org/ticket/8327#comment:92
<p>
Apply trac_8327_universal_cyclotomic_field-cs.patch
</p>
<p>
(for patchbot)
</p>
TicketdavidloefflerSat, 31 Mar 2012 16:02:00 GMTstatus changed
https://trac.sagemath.org/ticket/8327#comment:93
https://trac.sagemath.org/ticket/8327#comment:93
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Right, so now we have a patch which actually works, we can start testing it. And we find the following bug:
</p>
<pre class="wiki">----------------------------------------------------------------------
| Sage Version 5.0.beta11, Release Date: 2012-03-28 |
| Type notebook() for the GUI, and license() for information. |
----------------------------------------------------------------------
**********************************************************************
* *
* Warning: this is a prerelease version, and it may be unstable. *
* *
**********************************************************************
sage: z1 = E(3)
sage: z2 = UCF(CyclotomicField(3).gen())
sage: z1 == z2
False
sage: z1 + z2
---------------------------------------------------------------------------
AttributeError Traceback (most recent call last)
/home/masiao/<ipython console> in <module>()
/storage/masiao/sage-5.0.beta11/local/lib/python2.7/site-packages/sage/structure/element.so in sage.structure.element.RingElement.__add__ (sage/structure/element.c:11990)()
/storage/masiao/sage-5.0.beta11/local/lib/python2.7/site-packages/sage/structure/element.so in sage.structure.element.ModuleElement._add_ (sage/structure/element.c:8612)()
/storage/masiao/sage-5.0.beta11/local/lib/python2.7/site-packages/sage/rings/universal_cyclotomic_field/universal_cyclotomic_field.pyc in _add_(self, other)
890 E(12)^4 - E(12)^7 - E(12)^11
891 """
--> 892 n,m = self.field_order(),other.field_order()
893 l = cached_lcm( [ n, m ] )
894
/storage/masiao/sage-5.0.beta11/local/lib/python2.7/site-packages/sage/rings/universal_cyclotomic_field/universal_cyclotomic_field.pyc in field_order(self)
1020 3
1021 """
-> 1022 if bool(self.value._monomial_coefficients):
1023 return iter(self.value._monomial_coefficients).next()[0]
1024 else:
/storage/masiao/sage-5.0.beta11/local/lib/python2.7/site-packages/sage/structure/element.so in sage.structure.element.Element.__getattr__ (sage/structure/element.c:2919)()
/storage/masiao/sage-5.0.beta11/local/lib/python2.7/site-packages/sage/structure/parent.so in sage.structure.parent.getattr_from_other_class (sage/structure/parent.c:3329)()
AttributeError: 'sage.rings.number_field.number_field_element_quadratic.NumberFieldElement_quadratic' object has no attribute '_monomial_coefficients'
</pre><p>
This is even worse, since the element involved isn't cyclotomic in the first place:
</p>
<pre class="wiki">sage: K.<a> = NumberField(x^3 - 7)
sage: UCF(a)
a
sage: UCF(a) in UCF
True
</pre><p>
In fact UCF will attempt to construct an element from anything whatsoever:
</p>
<pre class="wiki">sage: UCF(None)
None
sage: UCF(QQ)
Rational Field
sage: UCF('hello')
'hello'
</pre><p>
The following are not great either:
</p>
<pre class="wiki">sage: UCF.has_coerce_map_from(CyclotomicField(3))
False
sage: QQbar.has_coerce_map_from(UCF)
False
</pre>
Ticketstumpc5Sat, 31 Mar 2012 17:36:48 GMT
https://trac.sagemath.org/ticket/8327#comment:94
https://trac.sagemath.org/ticket/8327#comment:94
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:93" title="Comment 93">davidloeffler</a>:
</p>
<blockquote class="citation">
<p>
In fact UCF will attempt to construct an element from anything whatsoever:
</p>
<pre class="wiki">sage: UCF(None)
None
sage: UCF(QQ)
Rational Field
sage: UCF('hello')
'hello'
</pre></blockquote>
<p>
I didn't implement the <code>__call__</code> method - what is the behaviour you would expect here?
</p>
<blockquote class="citation">
<p>
The following are not great either:
</p>
<pre class="wiki">sage: UCF.has_coerce_map_from(CyclotomicField(3))
False
</pre></blockquote>
<p>
how can I get a coerce from <code>CyclotomicField(n)</code> for all <code>n</code>?
</p>
<blockquote class="citation">
<pre class="wiki">sage: QQbar.has_coerce_map_from(UCF)
False
</pre></blockquote>
<p>
I fixed this.
</p>
TicketdavidloefflerSun, 01 Apr 2012 11:28:11 GMT
https://trac.sagemath.org/ticket/8327#comment:95
https://trac.sagemath.org/ticket/8327#comment:95
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:94" title="Comment 94">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
I didn't implement the <code>__call__</code> method - what is the behaviour you would expect here?
</p>
</blockquote>
<p>
Oh, I don't know, maybe the same behaviour as every other parent object in the Sage library? The default <code> __call__ </code> calls <code> _element_constructor_ </code>, which you should override, in order to create an element of self from the given arguments. This includes checking whether the given arguments are reasonable, and raising a TypeError otherwise.
</p>
<p>
You should also set the attribute wrapped_class for your element class to something other than the default (which is "object"), so the <code> __init__</code> it inherits from ElementWrapper knows what it's supposed to be wrapping and doesn't wrap obviously bogus objects as in the examples above.
</p>
<blockquote class="citation">
<p>
how can I get a coerce from <code>CyclotomicField(n)</code> for all <code>n</code>?
</p>
</blockquote>
<p>
I don't quite understand the question, but if you're going to implement a class and call it "universal cyclotomic field", surely it had better satisfy a universal property with respect to cyclotomic fields? If your question is "how do I recognise a cyclotomic field", you might find <code> isinstance(K, sage.rings.number_field.number_field.NumberField_cyclotomic) </code> or perhaps <code> sage.rings.number_field.number_field.is_CyclotomicField(K) </code> helpful. You should also check <code> K.coerce_embedding</code>, since it's possible to create cyclotomic fields with complex embeddings other than the standard one.
</p>
Ticketstumpc5Sun, 01 Apr 2012 12:34:48 GMT
https://trac.sagemath.org/ticket/8327#comment:96
https://trac.sagemath.org/ticket/8327#comment:96
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:95" title="Comment 95">davidloeffler</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:94" title="Comment 94">stumpc5</a>:
Oh, I don't know, maybe the same behaviour as every other parent object in the Sage library? The default <code> __call__ </code> calls <code> _element_constructor_ </code>, which you should override, in order to create an element of self from the given arguments. This includes checking whether the given arguments are reasonable, and raising a TypeError otherwise.
</p>
</blockquote>
<p>
The idea is that one should *not* construct elements of the universal cyclotomic field using <code>UCF(arg)</code> but only through the function <code>E</code>, e.g., <code>E(5)</code>. So <code>UCF(arg)</code> should only be called for coercion.
</p>
<p>
I could overwrite <code>_element_constructor_</code> by
</p>
<pre class="wiki">def _element_constructor_(self,arg):
raise TypeError, "No coercion found for %s"%str(arg)
</pre><p>
This would prevent <code>UCF</code> from ever constructing any element, and only using the coercion model to coerce objects if possible.
</p>
<blockquote class="citation">
<p>
You should also set the attribute wrapped_class for your element class to something other than the default (which is "object"), so the <code> __init__</code> it inherits from ElementWrapper knows what it's supposed to be wrapping and doesn't wrap obviously bogus objects as in the examples above.
</p>
</blockquote>
<p>
Do I see it right that this would then also be obsolete?
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
how can I get a coerce from <code>CyclotomicField(n)</code> for all <code>n</code>?
</p>
</blockquote>
<p>
I don't quite understand the question, but if you're going to implement a class and call it "universal cyclotomic field", surely it had better satisfy a universal property with respect to cyclotomic fields?
</p>
</blockquote>
<p>
Let me define one coercion:
</p>
<pre class="wiki">sage: from sage.rings.number_field.number_field_morphisms import NumberFieldEmbedding
sage: n = 5
sage: X = CyclotomicField(n)
sage: f = NumberFieldEmbedding(X,UCF,E(n))
sage: UCF.register_coercion(f)
sage: x = X.gen()
sage: UCF(x)
E(5)
</pre><p>
First, how can I register this coercion for all <code>n</code>? Second, are there further number fields I should have a coercion from?
</p>
TicketdavidloefflerSun, 01 Apr 2012 13:19:35 GMT
https://trac.sagemath.org/ticket/8327#comment:97
https://trac.sagemath.org/ticket/8327#comment:97
<p>
Aargh! This is so disastrously wrong I don't even know where to start. For heavens' sake, go and read the Sage reference manual -- there's a whole huge tutorial on how to implement coercions properly.
</p>
<p>
I think I've had enough of this ticket; perhaps someone else can take over "reviewing" it from here onwards.
</p>
Ticketstumpc5Sun, 01 Apr 2012 14:02:56 GMT
https://trac.sagemath.org/ticket/8327#comment:98
https://trac.sagemath.org/ticket/8327#comment:98
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:97" title="Comment 97">davidloeffler</a>:
</p>
<blockquote class="citation">
<p>
Aargh! This is so disastrously wrong I don't even know where to start. For heavens' sake, go and read the Sage reference manual -- there's a whole huge tutorial on how to implement coercions properly.
</p>
<p>
I think I've had enough of this ticket; perhaps someone else can take over "reviewing" it from here onwards.
</p>
</blockquote>
<p>
First, I consider this strongly unfriendly.
</p>
<p>
Second, after looking again at the coercion section of the reference manual, I still don't see what is so disastrously wrong (which obviously doesn't mean that it is not wrong), and how to tell a parent to register coercion from an infinite family of other parents.
</p>
TicketdavidloefflerSun, 01 Apr 2012 14:21:53 GMT
https://trac.sagemath.org/ticket/8327#comment:99
https://trac.sagemath.org/ticket/8327#comment:99
<p>
I'm sorry, please pardon my earlier outburst -- it was uncalled for, your message just caught me at a bad time.
</p>
<p>
See <a href="http://www.sagemath.org/doc/reference/coercion.html#methods-to-implement">http://www.sagemath.org/doc/reference/coercion.html#methods-to-implement</a>. The magic routines here are <code> _element_constructor_</code> and <code> _coerce_map_from_ </code>. In this case, you can make <code> UCF._coerce_map_from_(K) </code> return the natural morphism from K to self whenever K is a cyclotomic field (but you need to watch out for cyclotomic fields with non-standard embeddings).
</p>
Ticketstumpc5Sun, 01 Apr 2012 19:00:13 GMT
https://trac.sagemath.org/ticket/8327#comment:100
https://trac.sagemath.org/ticket/8327#comment:100
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:99" title="Comment 99">davidloeffler</a>:
</p>
<blockquote class="citation">
<p>
See <a href="http://www.sagemath.org/doc/reference/coercion.html#methods-to-implement">http://www.sagemath.org/doc/reference/coercion.html#methods-to-implement</a>. The magic routines here are
</p>
</blockquote>
<p>
<code> _coerce_map_from_ </code>
</p>
<p>
Thanks, I now see how I can do the coercion here - except that I do not know how to handle non-standard embeddings. I do not know how to test if a non-standard embedding is used, and if so, I also do not know how to coerce then into the <code>UCF</code>.
</p>
<p>
<code> _element_constructor_</code>
</p>
<p>
I want that an element of <code>UCF</code> is never constructed using the <code>__call__</code> method but only using the function <code>E</code>. In the 4 steps in <a href="http://www.sagemath.org/doc/reference/coercion.html#provided-methods">http://www.sagemath.org/doc/reference/coercion.html#provided-methods</a> <code>__call__</code>, I want the first 3 to proceed, and if no coercion is found, that an error is raised. The idea here is that <code>UCF</code> has an infinite number of generators, and the easiest to play with them seems to be the same way this is done in <code>GAP</code>; simply doing things like
</p>
<pre class="wiki">sage: E(3)
E(3)
sage: E(3)+E(4)
E(12)^4 - E(12)^7 - E(12)^11
</pre><p>
Also, I want to see
</p>
<pre class="wiki">sage: UCF(5)
5
sage: UCF(5).parent()
Universal Cyclotomic Field endowed with the Zumbroich basis
sage: X = CyclotomicField(6)
sage: UCF(X.gen())
-E(3)^2
</pre><p>
Is this generally wrong, or is the way I proposed to define <code>_element_constructor_</code> in my last comment just not the way to do that?
</p>
TicketchapotonTue, 20 Nov 2012 09:20:04 GMT
https://trac.sagemath.org/ticket/8327#comment:101
https://trac.sagemath.org/ticket/8327#comment:101
<p>
Ok, I am willing to try a review. I am not sure to be competent enough. We will see.
</p>
<p>
Let us first try to get a full green light from the bot. Could you please upload the latest version, rebased on a recent copy of sage ?
</p>
Ticketstumpc5Tue, 20 Nov 2012 09:22:35 GMT
https://trac.sagemath.org/ticket/8327#comment:102
https://trac.sagemath.org/ticket/8327#comment:102
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:101" title="Comment 101">chapoton</a>:
</p>
<blockquote class="citation">
<p>
Ok, I am willing to try a review. I am not sure to be competent enough. We will see.
</p>
<p>
Let us first try to get a full green light from the bot. Could you please upload the latest version, rebased on a recent copy of sage ?
</p>
</blockquote>
<p>
That would be really great! I will do that now (and also get the documentation into a good state-of-the-art shape), and let you know once the patch is ready for you to look at.
</p>
Ticketstumpc5Tue, 20 Nov 2012 10:27:31 GMTdependencies changed
https://trac.sagemath.org/ticket/8327#comment:103
https://trac.sagemath.org/ticket/8327#comment:103
<ul>
<li><strong>dependencies</strong>
changed from <em>#4539 #10771 #7980</em> to <em>#4539, #10771, #7980, #13727, #13728</em>
</li>
</ul>
Ticketstumpc5Tue, 20 Nov 2012 10:30:41 GMT
https://trac.sagemath.org/ticket/8327#comment:104
https://trac.sagemath.org/ticket/8327#comment:104
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:103" title="Comment 103">stumpc5</a>:
</p>
<p>
For a little better readability, I started by extracting two minor changes in other parts of Sage into two new tickets, <a class="closed ticket" href="https://trac.sagemath.org/ticket/13727" title="enhancement: Minor improvements for dict_addition (closed: fixed)">#13727</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/13728" title="enhancement: Implements some standard methods for fields (closed: fixed)">#13728</a>.
</p>
Ticketstumpc5Tue, 20 Nov 2012 11:57:49 GMTstatus, dependencies, description changed
https://trac.sagemath.org/ticket/8327#comment:105
https://trac.sagemath.org/ticket/8327#comment:105
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>dependencies</strong>
changed from <em>#4539, #10771, #7980, #13727, #13728</em> to <em>#4539, #10771, #7980, #9651, #13727, #13728</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/8327?action=diff&version=105">diff</a>)
</li>
</ul>
Ticketstumpc5Tue, 20 Nov 2012 11:59:06 GMTdependencies changed
https://trac.sagemath.org/ticket/8327#comment:106
https://trac.sagemath.org/ticket/8327#comment:106
<ul>
<li><strong>dependencies</strong>
changed from <em>#4539, #10771, #7980, #9651, #13727, #13728</em> to <em>#13727, #13728</em>
</li>
</ul>
Ticketstumpc5Tue, 20 Nov 2012 12:04:40 GMT
https://trac.sagemath.org/ticket/8327#comment:107
https://trac.sagemath.org/ticket/8327#comment:107
<p>
I uploaded a new version of the patch - let's see what the patchbot says.
</p>
<p>
@Fred: when you start reviewing the patch, please don't try to understand what happens in <code>sage/rings/universal_cyclotomic_field/universal_cyclotomic_field_c.pyx</code>. All actual arithmetic is done there, but very low-level (and I was - and still am - not at all an expert in implementing in cython). I hope it is convincing if the front end behaves as you expect and want it to behave, and is fairly quick.
</p>
Ticketstumpc5Tue, 20 Nov 2012 12:08:10 GMT
https://trac.sagemath.org/ticket/8327#comment:108
https://trac.sagemath.org/ticket/8327#comment:108
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:107" title="Comment 107">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
I uploaded a new version of the patch - let's see what the patchbot says.
</p>
</blockquote>
<p>
I don't quite understand what it is complaining about; some files that get rejected are not even contained in the patch. Any ideas what I do wrong?
</p>
TicketSimonKingTue, 20 Nov 2012 12:24:59 GMT
https://trac.sagemath.org/ticket/8327#comment:109
https://trac.sagemath.org/ticket/8327#comment:109
<p>
Really strange. For the record: The patchbot says
</p>
<pre class="wiki">applying trac_8327_universal_cyclotomic_field-cs.patch
patching file doc/en/reference/categories.rst
Hunk #1 FAILED at 9
1 out of 1 hunks FAILED -- saving rejects to file doc/en/reference/categories.rst.rej
patching file doc/en/reference/rings_numerical.rst
Hunk #1 FAILED at 26
1 out of 1 hunks FAILED -- saving rejects to file doc/en/reference/rings_numerical.rst.rej
patching file sage/categories/fields.py
</pre><p>
but if you search the string ".rst" in <a class="missing attachment">attachment:trac_8327_universal_cyclotomic_field-cs.patch</a>, you only find that <code>doc/en/reference/rings_standard.rst</code> is touched.
</p>
TicketchapotonTue, 20 Nov 2012 14:30:53 GMT
https://trac.sagemath.org/ticket/8327#comment:110
https://trac.sagemath.org/ticket/8327#comment:110
<p>
well, looks like a strange problem in the building of the doc. Maybe one could try a patch which does not touch the doc at all, just to see.
</p>
Ticketstumpc5Wed, 21 Nov 2012 08:47:19 GMT
https://trac.sagemath.org/ticket/8327#comment:111
https://trac.sagemath.org/ticket/8327#comment:111
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:110" title="Comment 110">chapoton</a>:
</p>
<blockquote class="citation">
<p>
well, looks like a strange problem in the building of the doc. Maybe one could try a patch which does not touch the doc at all, just to see.
</p>
</blockquote>
<p>
I now divides the patch in three parts, only the last touching the documentation...
</p>
Ticketstumpc5Wed, 21 Nov 2012 09:11:58 GMT
https://trac.sagemath.org/ticket/8327#comment:112
https://trac.sagemath.org/ticket/8327#comment:112
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:111" title="Comment 111">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:110" title="Comment 110">chapoton</a>:
</p>
<blockquote class="citation">
<p>
well, looks like a strange problem in the building of the doc. Maybe one could try a patch which does not touch the doc at all, just to see.
</p>
</blockquote>
<p>
I now divides the patch in three parts, only the last touching the documentation...
</p>
</blockquote>
<p>
Okay, it seems to work now -- I will take care now of the startup modules and the trailing whitespaces. Getting the coverage and no test failures will take a little longer (due to nasty cython functions that I can only test indirectly).
</p>
Ticketstumpc5Wed, 21 Nov 2012 11:12:42 GMTdependencies changed
https://trac.sagemath.org/ticket/8327#comment:113
https://trac.sagemath.org/ticket/8327#comment:113
<ul>
<li><strong>dependencies</strong>
changed from <em>#13727, #13728</em> to <em>#13727, #13728, #13734, #13735</em>
</li>
</ul>
Ticketstumpc5Wed, 21 Nov 2012 14:38:23 GMT
https://trac.sagemath.org/ticket/8327#comment:114
https://trac.sagemath.org/ticket/8327#comment:114
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:112" title="Comment 112">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
it seems to work now
</p>
</blockquote>
<p>
The patch is finally ready for review. The only issue with the patchbot is currently that I do load the module at startup - which is what I'd prefer (as long as no one else has a better idea of how to handle it).
</p>
<p>
Beside that, I added 100% coverage everywhere, and excluded 4 very small side tickets to be reviewed directly.
</p>
<p>
Cheers, Christian
</p>
TicketchapotonWed, 21 Nov 2012 16:52:06 GMT
https://trac.sagemath.org/ticket/8327#comment:115
https://trac.sagemath.org/ticket/8327#comment:115
<p>
let me start with a few simple remarks
</p>
<ul><li>why are we changing the import of hecke_modules in this patch ?
</li></ul><ul><li>the doc of prime_subfield has a mistake : ZZ should be QQ
</li></ul><ul><li>in from_dict, only one parameter is documented
</li></ul><ul><li>the docs of _eq_ and _ne_ are strange, and look like a copy and paste
</li></ul><ul><li>is_unit should be removed, as already implemented in the class of Field elements
</li></ul><ul><li>it would be worth adding some of the methods of QQbar elements : minpoly and abs maybe
</li></ul><ul><li>maybe consider a coercion of real elements to the field AA ?
</li></ul><ul><li>somewhere near the beginning of the doc, it should be explained the following important points
</li></ul><blockquote>
<p>
1 the field UCF is contained in the complex field
</p>
</blockquote>
<p>
</p>
<blockquote>
<p>
2 how to coerce z to QQbar : QQbar(z)
</p>
</blockquote>
<blockquote>
<p>
3 how to coerce z to the minimal cyclotomic field containing it : z.to_cyclotomic_field()
</p>
</blockquote>
<p>
That's all for today.
</p>
Ticketstumpc5Wed, 21 Nov 2012 19:11:31 GMTdependencies changed
https://trac.sagemath.org/ticket/8327#comment:116
https://trac.sagemath.org/ticket/8327#comment:116
<ul>
<li><strong>dependencies</strong>
changed from <em>#13727, #13728, #13734, #13735</em> to <em>#13727, #13728</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:115" title="Comment 115">chapoton</a>:
</p>
<p>
Thanks for your first look!
</p>
<ul><li>I deleted the two later dependencies, since they are not really necessary (I only needed them at some point to work with the universal cyclotomics).
</li></ul><ul><li>I took your remarks into account:
</li></ul><blockquote class="citation">
<ul><li>why are we changing the import of hecke_modules in this patch ?
</li></ul></blockquote>
<p>
because the import of hecke_modules and of universal_cyclotomic_field causes an import loop. Since we want to use <code>UCF</code>, it was easier and cleaner to lazily import hecke_modules.
</p>
<blockquote class="citation">
<ul><li>is_unit should be removed, as already implemented in the class of Field elements
</li></ul></blockquote>
<p>
the inherited <code>is_unit</code> method is not capturing the situation. It's implementation is
</p>
<pre class="wiki"> def is_unit(self):
if self == 1 or self == -1:
return True
raise NotImplementedError
</pre><blockquote class="citation">
<ul><li>it would be worth adding some of the methods of QQbar elements : minpoly and abs maybe
</li></ul></blockquote>
<p>
not yet done!
</p>
<blockquote class="citation">
<ul><li>maybe consider a coercion of real elements to the field AA ?
</li></ul></blockquote>
<p>
This is impossible, isn't it? Here is a quote from the documentation:
</p>
<p>
"Another consequence of the consistency condition is that coercions can only go from exact rings (e.g., the rationals QQ) to inexact rings (e.g., real numbers with a fixed precision RR), but not the other way around."
</p>
<p>
Or am I misunderstanding something?
</p>
<blockquote class="citation">
<blockquote>
<p>
1 the field UCF is contained in the complex field
</p>
</blockquote>
</blockquote>
<p>
This coercion is going through QQbar, so I only documented the later.
</p>
<p>
Best, Christian
</p>
TicketchapotonWed, 21 Nov 2012 20:02:54 GMT
https://trac.sagemath.org/ticket/8327#comment:117
https://trac.sagemath.org/ticket/8327#comment:117
<blockquote class="citation">
<blockquote class="citation">
<ul><li>is_unit should be removed, as already implemented in the class of Field elements
</li></ul></blockquote>
<p>
the inherited <code>is_unit</code> method is not capturing the situation. It's implementation is
</p>
<pre class="wiki"> def is_unit(self):
if self == 1 or self == -1:
return True
raise NotImplementedError
</pre></blockquote>
<p>
I do not understand : if one look at Ticket <a class="closed ticket" href="https://trac.sagemath.org/ticket/13728" title="enhancement: Implements some standard methods for fields (closed: fixed)">#13728</a>, the method is_unit does check that an element is not zero !
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>maybe consider a coercion of real elements to the field AA ?
</li></ul></blockquote>
<p>
This is impossible, isn't it? Here is a quote from the documentation:
</p>
<p>
"Another consequence of the consistency condition is that coercions can only go from exact rings (e.g., the rationals QQ) to inexact rings (e.g., real numbers with a fixed precision RR), but not the other way around."
</p>
<p>
Or am I misunderstanding something?
</p>
</blockquote>
<p>
Well, I do not know. The point is that we can currently use QQbar(z) for z in UCF. I wonder wether one could do AA(z) for z in UCF and real, because AA is just the set of real elements of QQbar. But maybe I do not understand something..
</p>
<pre class="wiki">sage: toto=E(6)
sage: QQbar(toto)
0.500000000000000? + 0.866025403784439?*I
sage: riri=QQbar(toto)
sage: riri in AA
False
sage: riri+riri.conjugate() in AA
True
sage: QQbar(toto+toto.conjugate()) in AA
True
sage: AA(riri+riri.conjugate()).parent()
Algebraic Real Field
</pre><blockquote class="citation">
<blockquote class="citation">
<blockquote>
<p>
1 the field UCF is contained in the complex field
</p>
</blockquote>
</blockquote>
<p>
This coercion is going through QQbar, so I only documented the later.
</p>
</blockquote>
<p>
Yes, But I rather meant the mathematical point : the field is defined as an embedded field, not as an abstract field..
</p>
Ticketstumpc5Wed, 21 Nov 2012 20:27:12 GMT
https://trac.sagemath.org/ticket/8327#comment:118
https://trac.sagemath.org/ticket/8327#comment:118
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:117" title="Comment 117">chapoton</a>:
</p>
<blockquote class="citation">
<p>
I do not understand : if one look at Ticket <a class="closed ticket" href="https://trac.sagemath.org/ticket/13728" title="enhancement: Implements some standard methods for fields (closed: fixed)">#13728</a>, the method is_unit does check that an element is not zero !
</p>
</blockquote>
<p>
That's right, but even though UCF knows that it is a field, its elements do not know that they are field elements. Do you happen to know how to solve that?
</p>
<blockquote class="citation">
<p>
Well, I do not know. The point is that we can currently use QQbar(z) for z in UCF. I wonder wether one could do AA(z) for z in UCF and real, because AA is just the set of real elements of QQbar. But maybe I do not understand something..
</p>
</blockquote>
<p>
(I thought you meant the other way round...) It was easy to implement the conversion to QQ and ZZ, and to set the coercion to QQbar. But I don't quite know how to do conversion to AA.
</p>
<pre class="wiki">sage: AA(QQbar(E(5) + E(5)^4))
0.618033988749895?
</pre><p>
works, but one currently has to pass through QQbar.
</p>
<blockquote class="citation">
<p>
Yes, But I rather meant the mathematical point : the field is defined as an embedded field, not as an abstract field..
</p>
</blockquote>
<p>
Okay, I will add a sentence in the beginning.
</p>
TicketnthieryWed, 21 Nov 2012 21:56:11 GMT
https://trac.sagemath.org/ticket/8327#comment:119
https://trac.sagemath.org/ticket/8327#comment:119
<p>
Thanks so much Frédéric for taking over the review! (and in general for all your reviewing work!)
</p>
<p>
As a general comment: we have been discussing the overall design with Christian quite some and it looked good. So you can focus on the details.
</p>
Ticketstumpc5Thu, 22 Nov 2012 08:58:10 GMT
https://trac.sagemath.org/ticket/8327#comment:120
https://trac.sagemath.org/ticket/8327#comment:120
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:115" title="Comment 115">chapoton</a>:
</p>
<blockquote class="citation">
<ul><li>it would be worth adding some of the methods of QQbar elements : minpoly and abs maybe
</li></ul></blockquote>
<p>
I added implementations of those, and I also improved the documentation here and there a bit.
</p>
TicketchapotonThu, 22 Nov 2012 20:41:34 GMT
https://trac.sagemath.org/ticket/8327#comment:121
https://trac.sagemath.org/ticket/8327#comment:121
<p>
A few more comments
</p>
<ul><li>this import is apparently not used :
</li></ul><p>
from sage.rings.complex_field import <a class="missing wiki">ComplexField?</a>
</p>
<ul><li>the sort of elements is backwards compared to the order for cyclotomic fields
<pre class="wiki">sage: v=UCF.random_element(7);v
-5*E(7) - 4*E(7)^3 + 4*E(7)^4 + E(7)^5
sage: v.to_cyclotomic_field()
zeta7^5 + 4*zeta7^4 - 4*zeta7^3 - 5*zeta7
</pre></li></ul><p>
maybe it would be better to go the same way ? but maybe this is the same as gap ?
</p>
<ul><li>Is the is_subring method necessary ? it is almost empty !
</li></ul><ul><li>in the element constructor, maybe one could answer by something like
</li></ul><p>
"no coercion found for elements of XXX"
</p>
<p>
with XXX the parent of the argument ? It would be more informative, no ?
</p>
<ul><li>maybe zumbroich_basis should be called zumbroich_basis_indices ? I would expect that something called zumbroich_basis would return elements of the field, which is not really the case. Well, this is not that important..
</li></ul><ul><li>why is the hash function test tagged with random ? does it depend on the computer or something like that ?
</li></ul><ul><li>in _invert_, one should rather raise a <a class="missing wiki">ZeroDivisionError?</a> when trying to invert 0 ?
</li></ul><ul><li>more generally, maybe one could try to avoid using assert, and better raise precise Exceptions ?
</li></ul><ul><li>I have not read seriously the cython part, but found a typo :
</li></ul><p>
inverse for the univeral
</p>
<p>
That's all for today
</p>
Ticketstumpc5Fri, 23 Nov 2012 08:16:35 GMT
https://trac.sagemath.org/ticket/8327#comment:122
https://trac.sagemath.org/ticket/8327#comment:122
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:121" title="Comment 121">chapoton</a>:
</p>
<blockquote class="citation">
<p>
A few more comments
</p>
</blockquote>
<p>
Thanks a lot!
</p>
<blockquote class="citation">
<ul><li>the sort of elements is backwards compared to the order for cyclotomic fields
</li></ul><p>
maybe it would be better to go the same way ? but maybe this is the same as gap ?
</p>
</blockquote>
<p>
I used the same order as gap (the increasing ordering on the exponents). I think I would prefer this ordering...
</p>
<blockquote class="citation">
<ul><li>Is the is_subring method necessary ? it is almost empty !
</li></ul></blockquote>
<p>
I don't quite remember where but I needed this (indeed trivially implemented) method at some point to not catch an error. Maybe it was when working with matrices over UCF?
</p>
<blockquote class="citation">
<p>
"no coercion found for elements of XXX"
</p>
</blockquote>
<p>
done, I had to distinguish between objects having a parent and those not having one.
</p>
<blockquote class="citation">
<ul><li>maybe zumbroich_basis should be called zumbroich_basis_indices ? I would expect that something called zumbroich_basis would return elements of the field, which is not really the case. Well, this is not that important..
</li></ul></blockquote>
<p>
done
</p>
<blockquote class="citation">
<ul><li>why is the hash function test tagged with random ? does it depend on the computer or something like that ?
</li></ul></blockquote>
<p>
If I am not mistaken, the hash is only unique in a given Sage session.
</p>
<blockquote class="citation">
<ul><li>in _invert_, one should rather raise a <a class="missing wiki">ZeroDivisionError?</a> when trying to invert 0 ?
</li><li>more generally, maybe one could try to avoid using assert, and better raise precise Exceptions ?
</li></ul></blockquote>
<p>
good point, done!
</p>
<p>
Best, Christian
</p>
TicketnthieryFri, 23 Nov 2012 08:40:16 GMT
https://trac.sagemath.org/ticket/8327#comment:123
https://trac.sagemath.org/ticket/8327#comment:123
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:122" title="Comment 122">stumpc5</a>:
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>why is the hash function test tagged with random ? does it depend on the computer or something like that ?
</li></ul></blockquote>
<p>
If I am not mistaken, the hash is only unique in a given Sage session.
</p>
</blockquote>
<p>
It is not required that the hash value of an object be unique across
Sage session. However the implementation usually guarantees this,
which is a good feature. In fact, most of the time in Sage, the hash
value only depends on the architecture (32bits/64bits) and when this
is the case, it is good to test it explicitly in order to get noticed
in case the hash value would change for some reason (which could be a
bug or a long distance consequence of something else), as in:
</p>
<blockquote>
<p>
sage: e = <a class="missing wiki">RootSystem?</a>(['A',2]).ambient_space()
sage: hash(e.simple_root(0))
-4601450286177489034 # 64-bit
549810038 # 32-bit
</p>
</blockquote>
<p>
Cheers,
</p>
<blockquote>
<p>
Nicolas
</p>
</blockquote>
TicketnthieryFri, 23 Nov 2012 08:44:55 GMT
https://trac.sagemath.org/ticket/8327#comment:124
https://trac.sagemath.org/ticket/8327#comment:124
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:121" title="Comment 121">chapoton</a>:
</p>
<blockquote class="citation">
<ul><li>more generally, maybe one could try to avoid using assert, and better raise precise Exceptions ?
</li></ul></blockquote>
<p>
Just a side note: as discussed on sage-combinat-devel, assertions are
preferable in the following situations:
</p>
<ul><li>For checking the internal state of the program
</li><li>When the caller should *not* catch the exception
</li><li>In time critical locations
</li></ul><p>
Beside, nothing prevents from writing a meaningful message in an
assertion check.
</p>
<p>
Cheers,
</p>
<blockquote>
<p>
Nicolas
</p>
</blockquote>
Ticketstumpc5Fri, 23 Nov 2012 08:48:28 GMT
https://trac.sagemath.org/ticket/8327#comment:125
https://trac.sagemath.org/ticket/8327#comment:125
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:123" title="Comment 123">nthiery</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:122" title="Comment 122">stumpc5</a>:
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>why is the hash function test tagged with random ? does it depend on the computer or something like that ?
</li></ul></blockquote>
<p>
If I am not mistaken, the hash is only unique in a given Sage session.
</p>
</blockquote>
<p>
It is not required that the hash value of an object be unique across
Sage session. However the implementation usually guarantees this,
which is a good feature. In fact, most of the time in Sage, the hash
value only depends on the architecture (32bits/64bits) and when this
is the case, it is good to test it explicitly in order to get noticed
in case the hash value would change for some reason
</p>
</blockquote>
<p>
Thanks for the clarification!
</p>
<p>
Do you also happen to know how to handle the real conversion as in <code>AA(QQbar(E(5) + E(5)^4))</code> in <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:118" title="Comment 118">comment:118</a> ?
</p>
TicketnthieryFri, 23 Nov 2012 09:41:09 GMT
https://trac.sagemath.org/ticket/8327#comment:126
https://trac.sagemath.org/ticket/8327#comment:126
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:125" title="Comment 125">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:123" title="Comment 123">nthiery</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:122" title="Comment 122">stumpc5</a>:
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>why is the hash function test tagged with random ? does it depend on the computer or something like that ?
</li></ul></blockquote>
<p>
If I am not mistaken, the hash is only unique in a given Sage session.
</p>
</blockquote>
<p>
It is not required that the hash value of an object be unique across
Sage session. However the implementation usually guarantees this,
which is a good feature. In fact, most of the time in Sage, the hash
value only depends on the architecture (32bits/64bits) and when this
is the case, it is good to test it explicitly in order to get noticed
in case the hash value would change for some reason
</p>
</blockquote>
<p>
Thanks for the clarification!
</p>
<p>
Do you also happen to know how to handle the real conversion as in <code>AA(QQbar(E(5) + E(5)^4))</code> in <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:118" title="Comment 118">comment:118</a> ?
</p>
</blockquote>
<p>
Did you try implementing the (partial) morphism from UCF to AA (using <a class="missing wiki">SetMorphism?</a> and the category of <a class="missing wiki">SetsWithPartialMaps?</a>), and register it as a conversion?
</p>
<p>
If yes, this could possibly be done during the initialization of UCF.
</p>
Ticketstumpc5Fri, 23 Nov 2012 10:06:11 GMT
https://trac.sagemath.org/ticket/8327#comment:127
https://trac.sagemath.org/ticket/8327#comment:127
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:126" title="Comment 126">nthiery</a>:
</p>
<blockquote class="citation">
<p>
Did you try implementing the (partial) morphism from UCF to AA (using <a class="missing wiki">SetMorphism?</a> and the category of <a class="missing wiki">SetsWithPartialMaps?</a>), and register it as a conversion?
</p>
</blockquote>
<p>
Good call, it works now! Thanks!
</p>
<p>
Btw: It would be nice to include this in the documentation, if not already done. At least if I google for <code>"sage SetMorphism SetsWithPartialMaps"</code>, I only get the file <code>root_space.py</code> which is perfectly fine to explain how to do it, but I only found it after you telling me what to look for.
</p>
Ticketstumpc5Fri, 23 Nov 2012 10:18:05 GMT
https://trac.sagemath.org/ticket/8327#comment:128
https://trac.sagemath.org/ticket/8327#comment:128
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:127" title="Comment 127">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:126" title="Comment 126">nthiery</a> :
</p>
</blockquote>
<blockquote class="citation">
<p>
Good call, it works now! Thanks!
</p>
</blockquote>
<p>
One more thing: do you also know how to do the same for the cyclotomic fields ? What I would need is something like
</p>
<pre class="wiki"> SetMorphism(
Hom(self, CyclotomicField(n), SetsWithPartialMaps()),
lambda elem: elem.to_cyclotomic_field()
).register_as_conversion()
</pre><p>
The problem is that the parameter <code>n</code> is not determined a priori, and I want the conversion to exist for any <code>n</code>.
</p>
TicketchapotonFri, 23 Nov 2012 10:21:42 GMT
https://trac.sagemath.org/ticket/8327#comment:129
https://trac.sagemath.org/ticket/8327#comment:129
<p>
two minor suggestions :
</p>
<ul><li>I would write "smallest subfield of the complex field" instead of "smallest subfield of the complex plane"
</li></ul><ul><li>I think one should explain somewhere at the beginning of the doc of E that the constant exp(1) can be found as the lowercase letter 'e'
</li></ul><p>
This is great that you managed to coerce to AA !
</p>
TicketnthieryFri, 23 Nov 2012 10:23:47 GMT
https://trac.sagemath.org/ticket/8327#comment:130
https://trac.sagemath.org/ticket/8327#comment:130
<p>
I am glad that it worked out!
</p>
<blockquote class="citation">
<p>
Btw: It would be nice to include this in the documentation, if not already done. At least if I google for <code>"sage SetMorphism SetsWithPartialMaps"</code>, I only get the file <code>root_space.py</code> which is perfectly fine to explain how to do it, but I only found it after you telling me what to look for.
</p>
</blockquote>
<p>
Indeed! A good spot might be Simon's tutorial worksheet on coercion:
</p>
<p>
<a class="ext-link" href="https://groups.google.com/forum/#!topic/sage-devel/CJEUEghnQx8"><span class="icon"></span>https://groups.google.com/forum/#!topic/sage-devel/CJEUEghnQx8</a>
</p>
<p>
Simon?
</p>
TicketnthieryFri, 23 Nov 2012 10:26:17 GMT
https://trac.sagemath.org/ticket/8327#comment:131
https://trac.sagemath.org/ticket/8327#comment:131
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:128" title="Comment 128">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
One more thing: do you also know how to do the same for the cyclotomic fields ? What I would need is something like
</p>
<pre class="wiki"> SetMorphism(
Hom(self, CyclotomicField(n), SetsWithPartialMaps()),
lambda elem: elem.to_cyclotomic_field()
).register_as_conversion()
</pre><p>
The problem is that the parameter <code>n</code> is not determined a priori, and I want the conversion to exist for any <code>n</code>.
</p>
</blockquote>
<p>
There is no perfect support for this. But I guess you could change the initializtion of <a class="missing wiki">CyclotomicField?</a> so that, when the n-th Cyclotomic field F is constructed, the partial conversion UCF->F is constructed and registered as a conversion (and possibly the reverse coercion as well).
</p>
Ticketstumpc5Fri, 23 Nov 2012 14:18:12 GMT
https://trac.sagemath.org/ticket/8327#comment:132
https://trac.sagemath.org/ticket/8327#comment:132
<p>
Hi --
</p>
<p>
I think we are now some big steps forward!
</p>
<ul><li>I took care of everything Fred was asking for, in particular the partial conversion to <code>AA</code>.
</li></ul><p>
and
</p>
<ul><li>I implemented conversion to the cyclotomic field ! There is still an issue left, which I cannot take care off. Namely, the embedding of the cyclotomic field into the complex numbers can be given in the constructor. But I didn't find a way to capture this embedding to send an element of the universal cyclotomics into the cyclotomic field AND preserve the embedding.
</li></ul><p>
Ready for another review!
</p>
<p>
Best, Christian
</p>
TicketchapotonFri, 23 Nov 2012 14:32:22 GMT
https://trac.sagemath.org/ticket/8327#comment:133
https://trac.sagemath.org/ticket/8327#comment:133
<p>
Hum, the patch has become so much bigger, and the html preview does no longer work. It seems that you are now touching many more files. This will make my work much more difficult. There seem to be a huge number of whitespace-removal, which is a bit perturbing. I am not happy with this latest version, indeed..
</p>
TicketnthieryFri, 23 Nov 2012 14:34:08 GMT
https://trac.sagemath.org/ticket/8327#comment:134
https://trac.sagemath.org/ticket/8327#comment:134
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:132" title="Comment 132">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
Hi --
</p>
<p>
I think we are now some big steps forward!
</p>
<ul><li>I took care of everything Fred was asking for, in particular the partial conversion to <code>AA</code>.
</li></ul><p>
and
</p>
<ul><li>I implemented conversion to the cyclotomic field ! There is still an issue left, which I cannot take care off. Namely, the embedding of the cyclotomic field into the complex numbers can be given in the constructor. But I didn't find a way to capture this embedding to send an element of the universal cyclotomics into the cyclotomic field AND preserve the embedding.
</li></ul></blockquote>
<p>
Cool!
</p>
<p>
I guess it's fair if you only provide conversions to-from those cyclotomic field that use the default embedding.
</p>
Ticketstumpc5Fri, 23 Nov 2012 14:45:01 GMT
https://trac.sagemath.org/ticket/8327#comment:135
https://trac.sagemath.org/ticket/8327#comment:135
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:133" title="Comment 133">chapoton</a>:
</p>
<blockquote class="citation">
<p>
Hum, the patch has become so much bigger, and the html preview does no longer work. It seems that you are now touching many more files. This will make my work much more difficult. There seem to be a huge number of whitespace-removal, which is a bit perturbing. I am not happy with this latest version, indeed..
</p>
</blockquote>
<p>
I do agree, I didn't check that carefully. All I did was to add a 5 line method to <code>rings/number_field/number_field.py</code> that does the conversion from UCF to CF.
</p>
<p>
In order to not get trailing white space errors, I removed them all (and this did in the end modify the complete file, unfortunately). Should I just leave the white spaces in order to keep the readability of the patch?
</p>
TicketchapotonFri, 23 Nov 2012 14:48:52 GMT
https://trac.sagemath.org/ticket/8327#comment:136
https://trac.sagemath.org/ticket/8327#comment:136
<p>
yes, please try to make a cleaner/shorter patch. I think it is good to never introduce new trailing whitespaces, but bad to remove them everywhere else in the files you touch, as this makes ugly patches. Maybe you can without much work restart from an untouched file rings/number_field/number_field.py ?
</p>
Ticketstumpc5Fri, 23 Nov 2012 14:58:48 GMT
https://trac.sagemath.org/ticket/8327#comment:137
https://trac.sagemath.org/ticket/8327#comment:137
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:136" title="Comment 136">chapoton</a>:
</p>
<blockquote class="citation">
<p>
yes, please try to make a cleaner/shorter patch. I think it is good to never introduce new trailing whitespaces, but bad to remove them everywhere else in the files you touch, as this makes ugly patches. Maybe you can without much work restart from an untouched file rings/number_field/number_field.py ?
</p>
</blockquote>
<p>
Do you accept it now as only a little change (which I actually consider a bit improvement), see my changes in <code>number_field.py</code> ?
</p>
TicketchapotonFri, 23 Nov 2012 15:04:43 GMT
https://trac.sagemath.org/ticket/8327#comment:138
https://trac.sagemath.org/ticket/8327#comment:138
<p>
yes, much better.
</p>
<ul><li>there is still the typo "the univeral cyclotomic"
</li></ul>
Ticketstumpc5Fri, 23 Nov 2012 15:08:17 GMT
https://trac.sagemath.org/ticket/8327#comment:139
https://trac.sagemath.org/ticket/8327#comment:139
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:138" title="Comment 138">chapoton</a>:
</p>
<blockquote class="citation">
<p>
yes, much better.
</p>
<ul><li>there is still the typo "the univeral cyclotomic"
</li></ul></blockquote>
<p>
Oh, I wasn't seeing this typo when you pointed at it in your comment. So I thought you were disliking the wordings, which I then changed.
</p>
<p>
It's fixed now and will be in the next version of the patch as soon as I have more things to change.
</p>
Ticketstumpc5Fri, 23 Nov 2012 16:05:34 GMT
https://trac.sagemath.org/ticket/8327#comment:140
https://trac.sagemath.org/ticket/8327#comment:140
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:139" title="Comment 139">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:138" title="Comment 138">chapoton</a>:
</p>
</blockquote>
<blockquote class="citation">
<p>
It's fixed now and will be in the next version of the patch as soon as I have more things to change.
</p>
</blockquote>
<p>
Okay, done -- I fixed this and added more documentation on getting to the cyclotomics.
</p>
TicketchapotonFri, 23 Nov 2012 16:25:15 GMT
https://trac.sagemath.org/ticket/8327#comment:141
https://trac.sagemath.org/ticket/8327#comment:141
<p>
Well, I think I do not have much more to say. I have not yet tested the new conversions, but I am confident that they work and will try them soon.
</p>
<p>
I think we can leave further improvements for later tickets. I have noted an error occuring when doing <a class="missing wiki">FractionField?</a>(<a class="missing wiki">PolynomialRing?</a>(UCF,'x')), which may need investigation. There is also the annoying fact that the elements do not know that they belong to a field. Given that the ticket has been waiting for a long time, I propose to postpone these minor questions to later tickets.
</p>
<p>
If the bot gives a green light, except for the startup module, I will give a positive review. This may have to wait for one week, as I am traveling next week.
</p>
<p>
One last thing : the following lines have to be modified, as this has been implemented :
</p>
<dl class="wiki"><dt>.. TODO</dt><dd>modify <a class="missing wiki">CyclotomicField?</a> to add a (partial) conversion from UCF
</dd></dl>
Ticketstumpc5Fri, 23 Nov 2012 16:46:46 GMT
https://trac.sagemath.org/ticket/8327#comment:142
https://trac.sagemath.org/ticket/8327#comment:142
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:141" title="Comment 141">chapoton</a>:
</p>
<p>
Thanks a lot for doing the final review!
</p>
<p>
I don't see how my patch might have influenced the doc failure on <code>sage/schemes/elliptic_curves/ell_number_field.py</code>, its tests pass on my machine (in roughly 45 sec, independent of my patch being applied or not).
</p>
TicketchapotonSat, 24 Nov 2012 15:33:18 GMT
https://trac.sagemath.org/ticket/8327#comment:143
https://trac.sagemath.org/ticket/8327#comment:143
<p>
Well, I do indeed have this failing test :
</p>
<p>
sage -t -force_lib "devel/sage/sage/schemes/elliptic_curves/ell_number_field.py"
Segmentation fault (core dumped)
</p>
<p>
with the patch applied, and it disappears when the patch is removed.
</p>
<p>
So I guess there <strong>is</strong> a problem somewhere..
</p>
TicketchapotonSat, 24 Nov 2012 15:53:24 GMT
https://trac.sagemath.org/ticket/8327#comment:144
https://trac.sagemath.org/ticket/8327#comment:144
<p>
I suspect (without a really good reason) that the choice of the letter E, which is a traditional name for elliptic curves, may be involved.. By the way, is it reasonable to use a single letter ? I myself do not like the fact that the letter R stands for something I do not care about.. Maybe one could ask the question on sage-devel : can we use the name E for the generators of the universal cyclotomic field ?
</p>
TicketnthierySat, 24 Nov 2012 18:54:20 GMT
https://trac.sagemath.org/ticket/8327#comment:145
https://trac.sagemath.org/ticket/8327#comment:145
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:144" title="Comment 144">chapoton</a>:
</p>
<blockquote class="citation">
<p>
I suspect (without a really good reason) that the choice of the letter E, which is a traditional name for elliptic curves, may be involved..
</p>
</blockquote>
<p>
Possibly, if E occurs explicitly in the doctest. Otherwise it should not interfer, as it is only imported by default in the intepreter and not in the sage sources. An alternative possibility is that some coercion involving cyclotomic fields now goes through UCF and causes trouble. You could rerun the failing tests with all the UCF coercions disabled.
</p>
<blockquote class="citation">
<p>
By the way, is it reasonable to use a single letter ? I myself do not like the fact that the letter R stands for something I do not care about.. Maybe one could ask the question on sage-devel : can we use the name E for the generators of the universal cyclotomic field ?
</p>
</blockquote>
<p>
That's a good point. The average Sage user is indeed different from the average GAP user. An alternative would be to use UCF.E, and to implement:
</p>
<pre class="wiki"> sage: UCF.inject_shorthands()
sage: E(3)
</pre><p>
(as for symmetric functions)
</p>
Ticketstumpc5Sun, 25 Nov 2012 18:53:16 GMT
https://trac.sagemath.org/ticket/8327#comment:146
https://trac.sagemath.org/ticket/8327#comment:146
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:143" title="Comment 143">chapoton</a>:
</p>
<blockquote class="citation">
<p>
sage -t -force_lib "devel/sage/sage/schemes/elliptic_curves/ell_number_field.py"
Segmentation fault (core dumped)
</p>
<p>
with the patch applied, and it disappears when the patch is removed.
</p>
</blockquote>
<p>
I just rechecked on my machine, and I get <code>All tests passed!</code> in both cases. So, what might be the difference between Frédérics and my machine? I am running 5.5.rc0 on a 64bit Linux machine.
</p>
<p>
I forwarded the question on the point of entry to the sage-devel mailing list, see <a class="ext-link" href="https://groups.google.com/forum/?fromgroups=#!topic/sage-devel/rJ6Y_-a__d8"><span class="icon"></span>https://groups.google.com/forum/?fromgroups=#!topic/sage-devel/rJ6Y_-a__d8</a>.
</p>
Ticketstumpc5Tue, 27 Nov 2012 13:58:02 GMTdescription changed
https://trac.sagemath.org/ticket/8327#comment:147
https://trac.sagemath.org/ticket/8327#comment:147
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/8327?action=diff&version=147">diff</a>)
</li>
</ul>
Ticketstumpc5Tue, 27 Nov 2012 13:59:35 GMT
https://trac.sagemath.org/ticket/8327#comment:148
https://trac.sagemath.org/ticket/8327#comment:148
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:146" title="Comment 146">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
I forwarded the question on the point of entry to the sage-devel mailing list, see <a class="ext-link" href="https://groups.google.com/forum/?fromgroups=#!topic/sage-devel/rJ6Y_-a__d8"><span class="icon"></span>https://groups.google.com/forum/?fromgroups=#!topic/sage-devel/rJ6Y_-a__d8</a>.
</p>
</blockquote>
<p>
I updated the patch according to changes discussed at sage-devel. The only thing missing is to decide how to handle possible embeddings.
</p>
Ticketstumpc5Fri, 30 Nov 2012 09:44:20 GMT
https://trac.sagemath.org/ticket/8327#comment:149
https://trac.sagemath.org/ticket/8327#comment:149
<p>
The patchbot gives a green light (for the very first time!), and the ticket is again for review. I did quite some changes since Frédéric looked at it, which were requested in the discussion on sage-devel. It would be nice if someone else would look at the changes as well!
</p>
<p>
Thanks, Christian
</p>
TicketnbruinFri, 30 Nov 2012 22:15:21 GMT
https://trac.sagemath.org/ticket/8327#comment:150
https://trac.sagemath.org/ticket/8327#comment:150
<p>
It looks like you have a memory leak in <code>cached_lcm</code>: that cache never gets cleared out. Can you argue that it will only be called with a limited variety of arguments so that you know the cache won't grow indefinitely?
</p>
<p>
Also, you should cache by set, not by tuple, since lcm isn't dependent on order or multiplicity. That can already save you a factor <em>n</em>! of memory.
</p>
<p>
(but really, unless you can argue the argument set here is going to be very limited, I don't think you can afford to build this cache)
</p>
TicketrobertwbSat, 01 Dec 2012 05:28:44 GMT
https://trac.sagemath.org/ticket/8327#comment:151
https://trac.sagemath.org/ticket/8327#comment:151
<p>
Lots of good code here!
</p>
<p>
Some comments:
</p>
<ul><li>Why is get_parent_of_embedding in the pyx file when it's only used in the py file?
</li></ul><ul><li>No need to lazily import universal_cyclotomic_field_c, it's already behind another lazy import.
</li></ul><ul><li><a class="missing wiki">CyclotomicField?</a>._coerce_map_from_ # TODO: use the embedding of other properly: you could use the method introduced in <a class="closed ticket" href="https://trac.sagemath.org/ticket/13765" title="defect: Cyclotomic embeddings should respect coercions. (closed: fixed)">#13765</a>, but if you don't want to do that then at least check it has the "standard" embedding sending the generator to the primitive root of unity CC or return None rather than silently producing wrong answers.
</li></ul><ul><li>Shouldn't _coerce_map_from_ accept QQ?
</li></ul><ul><li>Does "endowed with the Zumbroich basis" need to be part of the printed name? Also, what about a nice _latex_ representation?
</li></ul><ul><li>Using <code><></code> for != is deprecated, and I think goes away in python 3.
</li></ul><ul><li>Any reason not to inherit from Field?
</li></ul><ul><li>The <code>if bracket == ...</code> list might be better done with a dictionary mapping left to right, with a good exception on <a class="missing wiki">KeyError?</a>. Even better, IMHO, is to accept any even-character string (so one would pass "" or "[]" or "<<>>" or whatever one wanted).
</li></ul><ul><li><code>_gap_init = _repr_</code> Not now that we've allowed different printing options.
</li></ul><ul><li><code>__hash__</code> will fail on a 32-bit machine. Instead, test making a dict with UCF items then retrieving one.
</li></ul><ul><li>is <a class="missing wiki">ZumbroichBasisIndices?</a> really a Parent?
</li></ul><ul><li>I agree with nbruin, there's a lot of caching going on that never gets freed. I would suggest having capped size caches, if you determine that you really need them. (Also, for <a class="missing wiki">CyclotomicFieldCached?</a>, better to fix this to make <a class="missing wiki">CyclotomicField?</a> faster, possibly using a weakref cache or one of the unique parent factories.)
</li></ul>
TicketSimonKingSat, 01 Dec 2012 08:07:30 GMT
https://trac.sagemath.org/ticket/8327#comment:152
https://trac.sagemath.org/ticket/8327#comment:152
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:151" title="Comment 151">robertwb</a>:
</p>
<blockquote class="citation">
<ul><li>... (Also, for <a class="missing wiki">CyclotomicFieldCached?</a>, better to fix this to make <a class="missing wiki">CyclotomicField?</a> faster, possibly using a weakref cache or one of the unique parent factories.)
</li></ul></blockquote>
<p>
Small remark: <code>UniqueRepresentation</code> currently uses a permanent cache, but will hopefully use a weak cache at some point in the future - see <a class="closed ticket" href="https://trac.sagemath.org/ticket/12215" title="defect: Memleak in UniqueRepresentation, @cached_method (closed: fixed)">#12215</a>. I think the <code>UniqueFactory</code> already has a weak cache - but please verify.
</p>
TicketnthierySat, 01 Dec 2012 12:57:22 GMT
https://trac.sagemath.org/ticket/8327#comment:153
https://trac.sagemath.org/ticket/8327#comment:153
<blockquote>
<p>
Hi Robert,
</p>
</blockquote>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:151" title="Comment 151">robertwb</a>:
</p>
<blockquote class="citation">
<ul><li>Any reason not to inherit from Field?
</li></ul></blockquote>
<p>
I told him to :-)
</p>
<p>
We want to move away from the static class hierarchy (Monoid, Field,
...) when there is not a strong compelling reason to use it (i.e. it
makes a difference speed wise, e.g. for element arithmetic), since
that duplicates the far more flexible category hierarchy.
</p>
<p>
Cheers,
</p>
<blockquote>
<p>
Nicolas
</p>
</blockquote>
TicketrobertwbSat, 01 Dec 2012 18:07:09 GMT
https://trac.sagemath.org/ticket/8327#comment:154
https://trac.sagemath.org/ticket/8327#comment:154
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:153" title="Comment 153">nthiery</a>:
</p>
<blockquote class="citation">
<blockquote>
<p>
Hi Robert,
</p>
</blockquote>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:151" title="Comment 151">robertwb</a>:
</p>
<blockquote class="citation">
<ul><li>Any reason not to inherit from Field?
</li></ul></blockquote>
<p>
I told him to :-)
</p>
<p>
We want to move away from the static class hierarchy (Monoid, Field,
...) when there is not a strong compelling reason to use it (i.e. it
makes a difference speed wise, e.g. for element arithmetic), since
that duplicates the far more flexible category hierarchy.
</p>
</blockquote>
<p>
+1 to this. Then why is is_Field not obtained by default? I think there's performance benefit in moving to Ring at least, for multiplication (but I'd have to check that this is still the case).
</p>
Ticketstumpc5Sun, 02 Dec 2012 19:34:07 GMTdependencies changed
https://trac.sagemath.org/ticket/8327#comment:155
https://trac.sagemath.org/ticket/8327#comment:155
<ul>
<li><strong>dependencies</strong>
changed from <em>#13727, #13728</em> to <em>#13765</em>
</li>
</ul>
Ticketstumpc5Sun, 02 Dec 2012 19:47:15 GMT
https://trac.sagemath.org/ticket/8327#comment:156
https://trac.sagemath.org/ticket/8327#comment:156
<p>
Thanks for your comments!
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:150" title="Comment 150">nbruin</a>:
</p>
<blockquote class="citation">
<p>
(but really, unless you can argue the argument set here is going to be very limited, I don't think you can afford to build this cache)
</p>
</blockquote>
<p>
I removed this cache (and also some of the others)
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:151" title="Comment 151">robertwb</a>:
</p>
<blockquote class="citation">
<p>
Lots of good code here!
</p>
</blockquote>
<p>
some positive words are always appreciated, thanks!
</p>
<blockquote class="citation">
<ul><li>Why is get_parent_of_embedding in the pyx file when it's only used in the py file?
</li></ul></blockquote>
<p>
done
</p>
<blockquote class="citation">
<ul><li>No need to lazily import universal_cyclotomic_field_c, it's already behind another lazy import.
</li></ul></blockquote>
<p>
done
</p>
<blockquote class="citation">
<ul><li><a class="missing wiki">CyclotomicField?</a>._coerce_map_from_ # TODO: use the embedding of other properly: you could use the method introduced in <a class="closed ticket" href="https://trac.sagemath.org/ticket/13765" title="defect: Cyclotomic embeddings should respect coercions. (closed: fixed)">#13765</a>, but if you don't want to do that then at least check it has the "standard" embedding sending the generator to the primitive root of unity CC or return None rather than silently producing wrong answers.
</li></ul></blockquote>
<p>
What I now do is to take embeddings of <a class="missing wiki">CyclotomicFields?</a> into account, but do not coerce if the UCF doesn't have the standard embedding. Please recheck that it does what it properly...
</p>
<blockquote class="citation">
<ul><li>Shouldn't _coerce_map_from_ accept QQ?
</li></ul></blockquote>
<p>
This is captured in <code>_from_base_ring</code>.
</p>
<blockquote class="citation">
<ul><li>Does "endowed with the Zumbroich basis" need to be part of the printed name?
</li></ul></blockquote>
<p>
removed.
</p>
<blockquote class="citation">
<p>
Also, what about a nice _latex_ representation?
</p>
</blockquote>
<p>
still open.
</p>
<blockquote class="citation">
<ul><li>Using <code><></code> for != is deprecated, and I think goes away in python 3.
</li></ul></blockquote>
<p>
done.
</p>
<blockquote class="citation">
<ul><li>Any reason not to inherit from Field?
</li></ul></blockquote>
<p>
I now do inherit from Field and from <a class="missing wiki">FieldElement?</a>. This way, I got rid of several almost empty methods. If Nicolas prefers to have it again the other way round, we can as well undo that.
</p>
<blockquote class="citation">
<ul><li>The <code>if bracket == ...</code> list might be better done with a dictionary mapping left to right, with a good exception on <a class="missing wiki">KeyError?</a>. Even better, IMHO, is to accept any even-character string (so one would pass "" or "[]" or "<<>>" or whatever one wanted).
</li></ul></blockquote>
<p>
done.
</p>
<blockquote class="citation">
<ul><li><code>_gap_init = _repr_</code> Not now that we've allowed different printing options.
</li></ul></blockquote>
<p>
done.
</p>
<blockquote class="citation">
<ul><li><code>__hash__</code> will fail on a 32-bit machine. Instead, test making a dict with UCF items then retrieving one.
</li></ul></blockquote>
<p>
still open.
</p>
<blockquote class="citation">
<ul><li>is <a class="missing wiki">ZumbroichBasisIndices?</a> really a Parent?
</li></ul></blockquote>
<p>
I am not the specialist here, so feel free to modify this. Now, ZBI is a parent, and the tuples <code>(n,k)</code> are indeed elements thereof.
</p>
<blockquote class="citation">
<ul><li>I agree with nbruin, there's a lot of caching going on that never gets freed. I would suggest having capped size caches, if you determine that you really need them. (Also, for <a class="missing wiki">CyclotomicFieldCached?</a>, better to fix this to make <a class="missing wiki">CyclotomicField?</a> faster, possibly using a weakref cache or one of the unique parent factories.)
</li></ul></blockquote>
<p>
done.
</p>
<p>
Best, Christian
</p>
TicketnthierySun, 02 Dec 2012 20:23:41 GMT
https://trac.sagemath.org/ticket/8327#comment:157
https://trac.sagemath.org/ticket/8327#comment:157
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:156" title="Comment 156">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
I now do inherit from Field and from <a class="missing wiki">FieldElement?</a>. This way, I got rid of several almost empty methods. If Nicolas prefers to have it again the other way round, we can as well undo that.
</p>
</blockquote>
<p>
It would be best to move those almost empty methods to the Fields
category. That being said, I totally understand if you prefer to get
done with this patch without risking conflicts with ongoing category
development. Can you just leave a quick note in the code like:
</p>
<pre class="wiki"> .. TODO::
Remove the inheritance from Field and FieldElement as soon as
the methods X,Y,Z are implemented in the Fields category.
</pre><p>
Ideally you would also make a quick benchmark to check my guess that
inheriting from <a class="missing wiki">FieldElement?</a> does not make a speed difference.
</p>
<p>
Thanks!
</p>
<blockquote>
<p>
Nicolas
</p>
</blockquote>
Ticketstumpc5Mon, 03 Dec 2012 16:42:58 GMT
https://trac.sagemath.org/ticket/8327#comment:158
https://trac.sagemath.org/ticket/8327#comment:158
<blockquote class="citation">
<ul><li>Does "endowed with the Zumbroich basis" need to be part of the printed name? Also, what about a nice _latex_ representation?
</li></ul></blockquote>
<p>
This is actually taken care of in <a class="closed ticket" href="https://trac.sagemath.org/ticket/13734" title="enhancement: Implementation of latex method for ElementWrapper (closed: fixed)">#13734</a>
</p>
<blockquote class="citation">
<p>
<code>__hash__</code> will fail on a 32-bit machine. Instead, test making a dict with UCF items then retrieving one.
</p>
</blockquote>
<p>
??? What do you want me to do here? Are you talking about the hash for the parent of the elements?
</p>
TicketnbruinMon, 03 Dec 2012 17:13:06 GMT
https://trac.sagemath.org/ticket/8327#comment:159
https://trac.sagemath.org/ticket/8327#comment:159
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:158" title="Comment 158">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
??? What do you want me to do here? Are you talking about the hash for the parent of the elements?
</p>
</blockquote>
<p>
I only found hashes for elements in your patch. Hash values are supposed to be machine-length integers associated to an object, consistent within one session, equal for objects that compare as equal (and preferably not equal to much else) [sage has to violate this due to equality being too lax and not transitive in the presence of coercion] and very little structure otherwise. In particular, the actual numerical values returned don't matter. It's better to check the properties that actually matter than particular values that are produced (which should be dependent on 32/64 bits because otherwise you're creating unnecessarily weak hashes on 64 bit)
</p>
<p>
Don't test the actual value returned by hash, but test its functionality. Something like (do we have some example that exercises hashes appropriately?):
</p>
<pre class="wiki">sage: a = ...
sage: b = ...
sage: c = ...
sage: V = set([a,b])
sage: <definition for a> in V
true
sage: b in V
true
sage: c in V
false
</pre><p>
and perhaps test your hash function is not the constant function and has a reasonable distribution:
</p>
<pre class="wiki">sage: L = [<object> for i in [1..100]]
sage: set(hash(x) for x in L).len() >= 98
true
</pre><p>
You did mark the tests as <code>64 bit</code> which may or may not elicit the right action from the doctesting framework, but it would be way better if your test actually did work (and did test) on 32 bit as well, which you can do by testing functionality, not value.
</p>
Ticketstumpc5Mon, 03 Dec 2012 18:25:19 GMT
https://trac.sagemath.org/ticket/8327#comment:160
https://trac.sagemath.org/ticket/8327#comment:160
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:159" title="Comment 159">nbruin</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:158" title="Comment 158">stumpc5</a>:
</p>
</blockquote>
<p>
Thanks for the clarification!
</p>
<blockquote class="citation">
<p>
I only found hashes for elements in your patch.
</p>
</blockquote>
<p>
Right, but there is a <code>__hash__</code> function returning the id, that's why I wasn't quite sure... I now do test the hash by functionality, do you think it's better now?
</p>
<p>
Btw: As with the hash for elements in the CyclotomicField, it does not depend on the embedding of the parent. Do you think this might cause problems?
</p>
<p>
Finally, some functionalities become slower (e.g. matrix multiplication where addition or multiplication of UCF elements are done multiple times). Do you think, it is appropriate to cache (in the parent) the multipliciation or addition in UCF.Elements ?
</p>
TicketnbruinMon, 03 Dec 2012 18:56:38 GMT
https://trac.sagemath.org/ticket/8327#comment:161
https://trac.sagemath.org/ticket/8327#comment:161
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:160" title="Comment 160">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
Right, but there is a <code>__hash__</code> function returning the id, that's why I wasn't quite sure... I now do test the hash by functionality, do you think it's better now?
</p>
</blockquote>
<p>
I guess for parents that would be OK, because they are unique. Otherwise, most elements in sage can be equal to non-identical elements, so <code>id</code> doesn't make for a valid hash on them. Using <code>id</code> as a hash isn't consistent across sessions and won't survive pickling, but I'd hope that's OK.
</p>
<blockquote class="citation">
<p>
Btw: As with the hash for elements in the CyclotomicField, it does not depend on the embedding of the parent. Do you think this might cause problems?
</p>
</blockquote>
<p>
No. The constant function would make a valid hash function. It would just lead to horrible performance on dictionaries and sets.
</p>
<blockquote class="citation">
<p>
Finally, some functionalities become slower (e.g. matrix multiplication where addition or multiplication of UCF elements are done multiple times). Do you think, it is appropriate to cache (in the parent) the multipliciation or addition in UCF.Elements ?
</p>
</blockquote>
<p>
??? I hope you're not proposing caching by essentially building up addition and multiplication tables. That'll be horrible on memory and rarely result in speedup.
</p>
<p>
I'd say this is all optimization. First you want to have the functionality in (that'll already be great!). Once you have a stable platform, you can test where the bottlenecks are and optimize those, if required (with a minimum of caching).
</p>
TicketnthieryMon, 03 Dec 2012 20:31:26 GMT
https://trac.sagemath.org/ticket/8327#comment:162
https://trac.sagemath.org/ticket/8327#comment:162
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:159" title="Comment 159">nbruin</a>:
</p>
<blockquote class="citation">
<p>
Don't test the actual value returned by hash, but test its functionality. Something like (do we have some example that exercises hashes appropriately?):
</p>
<pre class="wiki">sage: a = ...
sage: b = ...
sage: c = ...
sage: V = set([a,b])
sage: <definition for a> in V
true
sage: b in V
true
sage: c in V
false
</pre><p>
and perhaps test your hash function is not the constant function and has a reasonable distribution:
</p>
<pre class="wiki">sage: L = [<object> for i in [1..100]]
sage: set(hash(x) for x in L).len() >= 98
true
</pre></blockquote>
<p>
Just a remark: those would make good candidates for a generic _test_hash method for parents.
</p>
TicketnthieryMon, 03 Dec 2012 20:33:41 GMT
https://trac.sagemath.org/ticket/8327#comment:163
https://trac.sagemath.org/ticket/8327#comment:163
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:160" title="Comment 160">stumpc5</a>:
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
I only found hashes for elements in your patch.
</p>
</blockquote>
<p>
Right, but there is a <code>__hash__</code> function returning the id, that's why I wasn't quite sure... I now do test the hash by functionality, do you think it's better now?
</p>
</blockquote>
<p>
That hash function for the parent, you inherit from <a class="missing wiki">UniqueRepresentation?</a>, right? In that case, there is no real need to retest it here.
</p>
<p>
Cheers,
</p>
<blockquote>
<p>
Nicolas
</p>
</blockquote>
TicketnbruinMon, 03 Dec 2012 20:54:25 GMT
https://trac.sagemath.org/ticket/8327#comment:164
https://trac.sagemath.org/ticket/8327#comment:164
<p>
Hi Christian,
</p>
<p>
Sorry that your code review is devolving into a kind of "how to vet newly introduced parents" experiment. Please don't take it personally! I think we're still getting useful contributions to Sage at large from this for now, though.
</p>
<p>
For doing good (generic) tests for how hash performs:
</p>
<pre class="wiki">sage: any( E(n) in L for n in [1001..2000] )
False
</pre><p>
This test doesn't really test the non-constantness of hashes. This code would still work if <code>hash(E(n))==0</code> would hold for all <code>n</code>. After equality of hash value, <code>in</code> will still fall back on <code>__eq__</code>, with one exception: identical objects will be found with <code>in</code>:
</p>
<pre class="wiki">sage: n = float(NaN)
sage: n == n
False
sage: n in set([n])
True
sage: m = float(NaN)
sage: m in set([n])
False
</pre><p>
It's an optimization that is necessary for python to have reasonable performance that leads to nasty consequences for objects that are not equal to themselves (which is really an odd edgecase anyway)
</p>
Ticketstumpc5Mon, 03 Dec 2012 21:04:27 GMT
https://trac.sagemath.org/ticket/8327#comment:165
https://trac.sagemath.org/ticket/8327#comment:165
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:164" title="Comment 164">nbruin</a>:
</p>
<blockquote class="citation">
<pre class="wiki">sage: any( E(n) in L for n in [1001..2000] )
False
</pre></blockquote>
<p>
Thanks, I deleted it...
</p>
TicketchapotonWed, 05 Dec 2012 13:39:50 GMT
https://trac.sagemath.org/ticket/8327#comment:166
https://trac.sagemath.org/ticket/8327#comment:166
<p>
Hello Christian,
</p>
<p>
in the file number_field.py, the documentation of the parameter bracket is wrong : you say
</p>
<p>
Can be <code></code>None<code></code>, <code></code>"("<code></code>, or <code></code>"["<code></code>, with
</p>
<p>
this is not consistent with the documentation in UCF, where it is an even-length chain
</p>
<p>
Do you think there are still important problems that need to be solved ? Maybe some caching questions ? And of course the patchbot is not happy with latest version.
</p>
<p>
Fred
</p>
Ticketstumpc5Wed, 05 Dec 2012 13:44:02 GMT
https://trac.sagemath.org/ticket/8327#comment:167
https://trac.sagemath.org/ticket/8327#comment:167
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:166" title="Comment 166">chapoton</a>:
</p>
<p>
Hi Fred,
</p>
<blockquote class="citation">
<p>
Can be <code></code>None<code></code>, <code></code>"("<code></code>, or <code></code>"["<code></code>, with
this is not consistent with the documentation in UCF, where it is an even-length chain
</p>
</blockquote>
<p>
Thanks for pointing this out!
</p>
<blockquote class="citation">
<p>
Do you think there are still important problems that need to be solved ?
</p>
</blockquote>
<p>
I fixed everything people were complaining about (or, to say it differently, where they saw possible improvements). I don't know if someone is still not happy with the current state...
</p>
<blockquote class="citation">
<p>
Maybe some caching questions ? And of course the patchbot is not happy with latest version.
</p>
</blockquote>
<p>
This is just a patchbot issue, I hope -- it says that 0 tests failed in several files.
</p>
<p>
I will update and upload the patch,
</p>
<p>
Christian
</p>
TicketSimonKingWed, 05 Dec 2012 14:21:28 GMT
https://trac.sagemath.org/ticket/8327#comment:168
https://trac.sagemath.org/ticket/8327#comment:168
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:167" title="Comment 167">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
This is just a patchbot issue, I hope -- it says that 0 tests failed in several files.
</p>
</blockquote>
<p>
That could be a severe and difficult-to-debug problem. On a different ticket, I also had "0 tests failed", and the problem was that Sage crashed while quitting: No test broke, but the test <em>framework</em> broke. It would make sense to repeat the failing tests, both "normally" and in verbose mode, and on different platforms.
</p>
TicketnbruinFri, 07 Dec 2012 20:31:21 GMT
https://trac.sagemath.org/ticket/8327#comment:169
https://trac.sagemath.org/ticket/8327#comment:169
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:167" title="Comment 167">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
This is just a patchbot issue, I hope -- it says that 0 tests failed in several files.
</p>
</blockquote>
<p>
In those cases, don't just look at the summary at the bottom.
There is a bit more info in the log file (use text search on the file name to find the relevant locations. The errors that arise are of the form:
</p>
<pre class="wiki"> File "/mnt/storage2TB/patchbot/Sage/sage-5.5.rc0/local/lib/python/site-packages/sage/rings/all.py", line 169, in <module>
lazy_import("sage.rings.universal_cyclotomic_field.all","*")
File "lazy_import.pyx", line 850, in sage.misc.lazy_import.lazy_import (sage/misc/lazy_import.c:5168)
File "lazy_import.pyx", line 900, in sage.misc.lazy_import.get_star_imports (sage/misc/lazy_import.c:5924)
EOFError
</pre><p>
so apparently the lazy import fails for some reason. If it's a real issue, perhaps it's due to the order in which the lazy imports get triggered? Otherwise it might just be a temporary issue with that particular patchbot.
</p>
Ticketstumpc5Sat, 08 Dec 2012 13:31:19 GMT
https://trac.sagemath.org/ticket/8327#comment:170
https://trac.sagemath.org/ticket/8327#comment:170
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:169" title="Comment 169">nbruin</a>:
</p>
<blockquote class="citation">
<p>
so apparently the lazy import fails for some reason. If it's a real issue, perhaps it's due to the order in which the lazy imports get triggered? Otherwise it might just be a temporary issue with that particular patchbot.
</p>
</blockquote>
<p>
And am I able to figure out if this is an issue which I can solve?
</p>
TicketnbruinSun, 09 Dec 2012 17:48:25 GMT
https://trac.sagemath.org/ticket/8327#comment:171
https://trac.sagemath.org/ticket/8327#comment:171
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:170" title="Comment 170">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
And am I able to figure out if this is an issue which I can solve?
</p>
</blockquote>
<p>
Well, given that you can't reproduce the errors (you have tried?) and that the patchbot reports errors in different tests each time, I'd assume a problem with the bot, unless evidence otherwise is supplied. Random <code>EOFError</code>s smell more like an I/O problem than an issue with the code.
</p>
Ticketstumpc5Mon, 10 Dec 2012 08:28:20 GMT
https://trac.sagemath.org/ticket/8327#comment:172
https://trac.sagemath.org/ticket/8327#comment:172
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:171" title="Comment 171">nbruin</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:170" title="Comment 170">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
And am I able to figure out if this is an issue which I can solve?
</p>
</blockquote>
<p>
Well, given that you can't reproduce the errors (you have tried?)
</p>
</blockquote>
<p>
Sorry for still being (maybe too) ignorant concerning these things: can I do anything other than starting sage and running the doctests?
</p>
TicketchapotonTue, 11 Dec 2012 20:58:39 GMT
https://trac.sagemath.org/ticket/8327#comment:173
https://trac.sagemath.org/ticket/8327#comment:173
<p>
What a boring problem, we are so close to finishing this patch ! And there is apparently only one patchbot around ! Nicolas, would it be possible to have a patchbot running for the combinat patches on one server somewhere ?
</p>
<p>
Some stupid ideas :
</p>
<ul><li>Maybe you could try to put back dependencies to <a class="closed ticket" href="https://trac.sagemath.org/ticket/13727" title="enhancement: Minor improvements for dict_addition (closed: fixed)">#13727</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/13728" title="enhancement: Implements some standard methods for fields (closed: fixed)">#13728</a> ? probably not a good idea, as they now also fail..
</li></ul><ul><li>try to trigger another run of the bot. I do not know how to do that. A priori, just asking by the following line is not enough.
</li></ul><p>
apply trac_8327-universal_cyclotomic_field-cs.patch
</p>
TicketnbruinTue, 11 Dec 2012 21:52:06 GMT
https://trac.sagemath.org/ticket/8327#comment:174
https://trac.sagemath.org/ticket/8327#comment:174
<p>
You can "kick" the patchbot by putting <code>?kick</code> after the URL of the ticket page at the patchbot. This has already happened and the errors keep shifting (look at the last couple of reports).
</p>
<p>
I suspect it's due to the lazy import (that's where the error occurs). I have no idea whether the lazy import here is done inappropriately, whether there's a subtle bug in lazy import that gets triggered here, or whether the patchbot has troubles that just happen to be triggered by this patch (sounds unlikely).
</p>
<p>
Since two of these three can be avoided by not doing a lazy import, that's probably the way to go if you want the ticket merged ASAP. Then "lazification" can be done on a separate ticket. That or someone referees the ticket with "works for me" and puts on a positive review, ignoring the bot.
</p>
TicketnbruinWed, 12 Dec 2012 19:04:32 GMT
https://trac.sagemath.org/ticket/8327#comment:175
https://trac.sagemath.org/ticket/8327#comment:175
<p>
See issue <a class="closed ticket" href="https://trac.sagemath.org/ticket/13826" title="defect: Race condition in star_imports cache (closed: fixed)">#13826</a>. I'd recommend to just ignore the intermittent patchbot failures. Presently, "parallel doctesting with .sage and tmp on different filesystems is not supported". The linked ticket will fix that. I don't think there is a need to make this ticket dependent on it. Someone should review this ticket, since it seems to be good to go for the rest.
</p>
TicketvbraunWed, 12 Dec 2012 19:16:45 GMTdependencies changed
https://trac.sagemath.org/ticket/8327#comment:176
https://trac.sagemath.org/ticket/8327#comment:176
<ul>
<li><strong>dependencies</strong>
changed from <em>#13765</em> to <em>#13765, #13826</em>
</li>
</ul>
TicketvbraunWed, 12 Dec 2012 19:18:08 GMT
https://trac.sagemath.org/ticket/8327#comment:177
https://trac.sagemath.org/ticket/8327#comment:177
<p>
I just want to test <a class="closed ticket" href="https://trac.sagemath.org/ticket/13826" title="defect: Race condition in star_imports cache (closed: fixed)">#13826</a>. Feel free to give this ticket a positive review. Human intelligence still trumps automated tests, you know ;-)
</p>
TicketnthieryThu, 13 Dec 2012 09:06:55 GMT
https://trac.sagemath.org/ticket/8327#comment:178
https://trac.sagemath.org/ticket/8327#comment:178
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:173" title="Comment 173">chapoton</a>:
</p>
<blockquote class="citation">
<p>
Nicolas, would it be possible to have a patchbot running for the combinat patches on one server somewhere ?
</p>
</blockquote>
<p>
Yes, that's indeed the plan and the purpose of the combinat server! I
wanted to work on it but it's buried in my todo pile; so please anyone
take over the task! Volker has posted instructions recently and it
seemed fairly straightforward.
</p>
<p>
Cheers,
</p>
<blockquote>
<p>
Nicolas
</p>
</blockquote>
TicketvbraunThu, 13 Dec 2012 14:01:35 GMT
https://trac.sagemath.org/ticket/8327#comment:179
https://trac.sagemath.org/ticket/8327#comment:179
<p>
Robert Bradshaw wrote the patchbot and posted instructions, not me. Everybody can run their own patchbot, its easy...
</p>
Ticketstumpc5Mon, 07 Jan 2013 12:35:38 GMT
https://trac.sagemath.org/ticket/8327#comment:180
https://trac.sagemath.org/ticket/8327#comment:180
<p>
Could someone please investigate what is going wrong now? I do
</p>
<pre class="wiki">lazy_import("sage.rings.universal_cyclotomic_field.all","*")
</pre><p>
in <code>sage.rings.all</code> but nevertheless, it seems that the UCF and also all the numpy stuff in the pyx file is imported at startup. When I do the import of <code>sage.rings.universal_cyclotomic_field.all</code> directly but then do the lazy import of <code>UniversalCyclotomicField</code> in there, everything runs as expected.
</p>
<p>
Is this something not treated properly in <a class="closed ticket" href="https://trac.sagemath.org/ticket/13826" title="defect: Race condition in star_imports cache (closed: fixed)">#13826</a> ?
</p>
<p>
Thanks, Christian
</p>
TicketvbraunMon, 07 Jan 2013 13:22:39 GMT
https://trac.sagemath.org/ticket/8327#comment:181
https://trac.sagemath.org/ticket/8327#comment:181
<p>
What is your problem, precisely? The lazy import does work:
</p>
<pre class="wiki">sage: type(UniversalCyclotomicField)
<type 'sage.misc.lazy_import.LazyImport'>
</pre><p>
I also don't see any instances being created elsewhere during Sage startup.
</p>
Ticketstumpc5Mon, 07 Jan 2013 14:04:47 GMT
https://trac.sagemath.org/ticket/8327#comment:182
https://trac.sagemath.org/ticket/8327#comment:182
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:181" title="Comment 181">vbraun</a>:
</p>
<blockquote class="citation">
<p>
What is your problem, precisely?
</p>
</blockquote>
<p>
When I do the lazy import of <code>sage.rings.universal_cyclotomic_field.all.*</code> in <code>sage.rings.all</code> (as in the current version of the patch on trac), I get
</p>
<pre class="wiki">$ sage -t sage/tests/startup.py
sage -t "devel/sage-UCF/sage/tests/startup.py"
**********************************************************************
File "/home/stumpc5/progs/sage-5.5.rc0/devel/sage-UCF/sage/tests/startup.py", line 17:
sage: sage0("'numpy' in sys.modules")
Expected:
False
Got:
True
**********************************************************************
</pre><p>
and this test doesn't fail if I do the lazy import of <code>UniversalCyclotomicField</code> within <code>sage.rings.universal_cyclotomic_field.all</code>. Numpy is imported in the cython file for the UCF, and seems to be imported on startup when doing
</p>
<pre class="wiki">lazy_import("sage.rings.universal_cyclotomic_field.all","*")
</pre><p>
but not when doing
</p>
<pre class="wiki">lazy_import("sage.rings.universal_cyclotomic_field.universal_cyclotomic_field","UniversalCyclotomicField")
</pre><p>
Do you know what goes wrong there?
</p>
TicketnbruinMon, 07 Jan 2013 17:24:48 GMT
https://trac.sagemath.org/ticket/8327#comment:183
https://trac.sagemath.org/ticket/8327#comment:183
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:182" title="Comment 182">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
Numpy is imported in the cython file for the UCF, and seems to be imported on startup when doing
</p>
<pre class="wiki">lazy_import("sage.rings.universal_cyclotomic_field.all","*")
</pre><p>
but not when doing
</p>
<pre class="wiki">lazy_import("sage.rings.universal_cyclotomic_field.universal_cyclotomic_field","UniversalCyclotomicField")
</pre></blockquote>
<p>
Have you taken into account that the first time running, lazy star imports are not lazy at all? It executes the actual import to cache which symbols get defined by the star import (the files in <code>.sage/cache</code>). I think this cache presently always gets deleted after <code>sage -b</code>. If testing <code>startup.py</code> is the first sage run after that, you cannot expect lazy behaviour.
</p>
<p>
This caching strategy has its flaws, but presently we'll have to live with it.
</p>
Ticketstumpc5Mon, 07 Jan 2013 17:50:20 GMT
https://trac.sagemath.org/ticket/8327#comment:184
https://trac.sagemath.org/ticket/8327#comment:184
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:183" title="Comment 183">nbruin</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:182" title="Comment 182">stumpc5</a>:
</p>
</blockquote>
<blockquote class="citation">
<p>
Have you taken into account that the first time running, lazy star imports are not lazy at all? It executes the actual import to cache which symbols get defined by the star import (the files in <code>.sage/cache</code>). I think this cache presently always gets deleted after <code>sage -b</code>. If testing <code>startup.py</code> is the first sage run after that, you cannot expect lazy behaviour.
</p>
<p>
This caching strategy has its flaws, but presently we'll have to live with it.
</p>
</blockquote>
<p>
Thanks for the explanation -- what am I now supposed to do to get things in order? Should I keep the lazy star import and change doctest in startup.py, or should I rather not do the star import?
</p>
TicketnbruinMon, 07 Jan 2013 18:39:11 GMT
https://trac.sagemath.org/ticket/8327#comment:185
https://trac.sagemath.org/ticket/8327#comment:185
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:184" title="Comment 184">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
Thanks for the explanation -- what am I now supposed to do to get things in order? Should I keep the lazy star import and change doctest in startup.py, or should I rather not do the star import?
</p>
</blockquote>
<p>
Well, if two consecutive runs still show the same behaviour, it's not the caching. Anyway, if you can avoid star-import altogether, caching doesn't come into play at all. I think it's more likely anyway that after the lazy import, something investigates the namespace in which the lazy import was done to such detail that it triggers the actual import, e.g., a <code>from module.name import *</code> when <code>module.name</code> contains a lazy import may well trigger actual importation (I haven't checked).
</p>
<p>
You'll have to avoid importing numpy on startup, since that has caused big startup delays previously (hence that doctest).
</p>
<p>
I think the main thing you're finding now is that we don't have much experience in how to use lazy import in more complicated scenarios, in particular in the whole <code>from ...all import *</code> scenarios that sage seems to use everywhere upon startup.
</p>
Ticketstumpc5Mon, 07 Jan 2013 19:18:32 GMTdependencies changed
https://trac.sagemath.org/ticket/8327#comment:186
https://trac.sagemath.org/ticket/8327#comment:186
<ul>
<li><strong>dependencies</strong>
changed from <em>#13765, #13826</em> to <em>#13765</em>
</li>
</ul>
TicketchapotonThu, 10 Jan 2013 20:44:52 GMTstatus changed
https://trac.sagemath.org/ticket/8327#comment:187
https://trac.sagemath.org/ticket/8327#comment:187
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
Ok, the bot is happy at last.
</p>
<p>
I am giving a positive review.
</p>
<p>
Cheers !
</p>
TicketjdemeyerThu, 10 Jan 2013 21:26:38 GMTmilestone changed
https://trac.sagemath.org/ticket/8327#comment:188
https://trac.sagemath.org/ticket/8327#comment:188
<ul>
<li><strong>milestone</strong>
changed from <em>sage-5.6</em> to <em>sage-pending</em>
</li>
</ul>
<p>
Please fill in the Reviewer field.
</p>
TicketchapotonThu, 10 Jan 2013 22:15:16 GMTreviewer set
https://trac.sagemath.org/ticket/8327#comment:189
https://trac.sagemath.org/ticket/8327#comment:189
<ul>
<li><strong>reviewer</strong>
set to <em>Frédéric Chapoton</em>
</li>
</ul>
Ticketstumpc5Fri, 11 Jan 2013 08:10:28 GMT
https://trac.sagemath.org/ticket/8327#comment:190
https://trac.sagemath.org/ticket/8327#comment:190
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:189" title="Comment 189">chapoton</a>:
</p>
<p>
Thanks a lot, Fred, and also to the others for all the contributions!
</p>
Ticketstumpc5Sat, 26 Jan 2013 21:18:56 GMTmilestone changed
https://trac.sagemath.org/ticket/8327#comment:191
https://trac.sagemath.org/ticket/8327#comment:191
<ul>
<li><strong>milestone</strong>
changed from <em>sage-pending</em> to <em>sage-5.7</em>
</li>
</ul>
TicketjdemeyerWed, 30 Jan 2013 17:38:55 GMTstatus changed
https://trac.sagemath.org/ticket/8327#comment:192
https://trac.sagemath.org/ticket/8327#comment:192
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
There is a lot of
</p>
<pre class="wiki">except:
</pre><p>
in the patch. Don't use this, as it will catch all exceptions, including stuff like <code>KeyboardInterrupt</code>. If you really want a catch-all, use
</p>
<pre class="wiki">except StandardError:
</pre><p>
but try to use specific exceptions where applicable.
</p>
Ticketstumpc5Wed, 30 Jan 2013 21:22:09 GMTstatus changed
https://trac.sagemath.org/ticket/8327#comment:193
https://trac.sagemath.org/ticket/8327#comment:193
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:192" title="Comment 192">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
There is a lot of
</p>
<pre class="wiki">except:
</pre><p>
in the patch.
</p>
</blockquote>
<p>
Thanks for the comment, I fixed this by removing some of the <code>except</code>'s, and used <code>KeyError</code>'s in two remaining places. I also fixed a bug, namely
</p>
<pre class="wiki">sage: UCF(0).is_rational()
False
</pre><p>
and added the doctests rechecking this. Since the patchbot doesn't like to apply <a class="closed ticket" href="https://trac.sagemath.org/ticket/13765" title="defect: Cyclotomic embeddings should respect coercions. (closed: fixed)">#13765</a> (which was merged in 5.7.beta2), I wonder if you could have another look and either rebase the patch, or - if you don't plan to merge it - to wait until I have a new version compiled to rebase it myself.
</p>
<p>
Thanks, Christian
</p>
TicketjdemeyerWed, 30 Jan 2013 21:28:35 GMT
https://trac.sagemath.org/ticket/8327#comment:194
https://trac.sagemath.org/ticket/8327#comment:194
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:193" title="Comment 193">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
I wonder if you could have another look and either rebase the patch
</p>
</blockquote>
<p>
Why should it be rebased? The fact that the patchbot is struggling is a bug with the patchbot, I don't think this ticket is to blame.
</p>
<p>
Anyway, I don't want to review this ticket (simply because I cannot review every ticket that I put to needs_work), but it should be easy for the original reviewer. Frédéric, can you do it?
</p>
Ticketstumpc5Wed, 30 Jan 2013 21:37:11 GMT
https://trac.sagemath.org/ticket/8327#comment:195
https://trac.sagemath.org/ticket/8327#comment:195
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:194" title="Comment 194">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:193" title="Comment 193">stumpc5</a>:
Anyway, I don't want to review this ticket (simply because I cannot review every ticket that I put to needs_work)
</p>
</blockquote>
<p>
Sorry, I didn't want to say you should actually review the patch again (though my message sounded like that), but only to possibly rebase it in case this would be the only modification needed to finally get this patch merged...
</p>
TicketchapotonThu, 31 Jan 2013 09:18:54 GMT
https://trac.sagemath.org/ticket/8327#comment:196
https://trac.sagemath.org/ticket/8327#comment:196
<p>
There remains 2 raise with the old syntax
</p>
<pre class="wiki">raise ValueError, "n (=%s) must be a positive integer"%n
</pre><pre class="wiki">raise TypeError, "Embedding (type %s) must be an element." % type(embedding)
</pre><p>
Please convert them to the Python 3 syntax.
</p>
TicketvbraunThu, 31 Jan 2013 09:34:36 GMT
https://trac.sagemath.org/ticket/8327#comment:197
https://trac.sagemath.org/ticket/8327#comment:197
<p>
On the topic of raising exceptions: start lower-case (and don't form complete sentences). Moreover, the coercion system will often feed you with invalid data to figure out what you can and cannot do. Hence it is best to avoid unnecessary work like creating string representations and concatenating them to form an exception message. Just raise <code>TypeError('x must be an element')</code>.
</p>
TicketjdemeyerThu, 31 Jan 2013 09:53:27 GMT
https://trac.sagemath.org/ticket/8327#comment:198
https://trac.sagemath.org/ticket/8327#comment:198
<p>
Another possible complaint about this patch is that it uses quite some Cython code but always without <a class="ext-link" href="http://sagemath.org/doc/developer/coding_in_cython.html#interrupt-and-signal-handling"><span class="icon"></span>sig_on()/sig_off()</a>. As a consequence, this code might not be interruptible on CTRL-C. I don't know to what extent this is a problem, I'm just pointing it out. Anyway, it can't hurt to put some <code>sig_check()</code> calls inside the loops.
</p>
Ticketstumpc5Thu, 31 Jan 2013 20:08:50 GMT
https://trac.sagemath.org/ticket/8327#comment:199
https://trac.sagemath.org/ticket/8327#comment:199
<p>
A general comment concerning several complaints on this ticket from various directions: If you ask mathematicians to contribute to Sage, you get mathematicians writing code and code that is not necessarily perfect. This does have disadvantages! But please don't blame them (i.e. me here) for not being perfect programmers!
</p>
<p>
Beside that, I fixed the complaint concerning error messages and their Python 3 syntax, but I am not going to go into interruption handling in cython. This can be handled in another ticket by someone more advanced.
</p>
<p>
Finally: I already spent much too much time on this ticket (i.e., I implemented the stuff I actually needed for my research 2 years ago, all further hours and days were not spent anymore for my primary research). I would very much appreciate if we could get this ticket into Sage, and then solve further issues in other patches.
</p>
TicketjdemeyerThu, 31 Jan 2013 20:16:20 GMTowner set
https://trac.sagemath.org/ticket/8327#comment:200
https://trac.sagemath.org/ticket/8327#comment:200
<ul>
<li><strong>owner</strong>
changed from <em>(none)</em> to <em>jdemeyer</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:199" title="Comment 199">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
I am not going to go into interruption handling in cython.
</p>
</blockquote>
<p>
No problem, note that I purposely did not set the status to needs_work for this. Your code is certainly not the only part of Sage with this problem.
</p>
<blockquote class="citation">
<p>
Finally: I already spent much too much time on this ticket (i.e., I implemented the stuff I actually needed for my research 2 years ago, all further hours and days were not spent anymore for my primary research).
</p>
</blockquote>
<p>
It's simply a fact that going from proof-of-concept-research-code to actual good code to be included in Sage is a non-trivial step. You could say it's unfortunate, but that's the way it is.
</p>
TicketvbraunThu, 31 Jan 2013 21:18:35 GMT
https://trac.sagemath.org/ticket/8327#comment:201
https://trac.sagemath.org/ticket/8327#comment:201
<p>
I actually do think the code here is pretty good, so please don't take everything as a criticism. I think this ticket is good to go, I just wanted to wait and see if the recently updated patchbot fares better at running it...
</p>
TicketjdemeyerFri, 01 Feb 2013 07:07:45 GMTstatus changed
https://trac.sagemath.org/ticket/8327#comment:202
https://trac.sagemath.org/ticket/8327#comment:202
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Now that you have changed the exceptions, also the doctests for the exceptions need to changed. I see various failures along the lines of
</p>
<pre class="wiki">**********************************************************************
File "/release/merger/sage-5.7.beta3/devel/sage-main/sage/rings/universal_cyclotomic_field/universal_cyclotomic_field.py", line 1755:
sage: E(6).galois_conjugates(5)
Expected:
Traceback (most recent call last):
...
ValueError: the given integer (5) is not a multiple of the field order of -E(3)^2
Got:
Traceback (most recent call last):
File "/release/merger/sage-5.7.beta3/local/bin/ncadoctest.py", line 1231, in run_one_test
self.run_one_example(test, example, filename, compileflags)
File "/release/merger/sage-5.7.beta3/local/bin/sagedoctest.py", line 38, in run_one_example
OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
File "/release/merger/sage-5.7.beta3/local/bin/ncadoctest.py", line 1172, in run_one_example
compileflags, 1) in test.globs
File "<doctest __main__.example_61[10]>", line 1, in <module>
E(Integer(6)).galois_conjugates(Integer(5))###line 1755:
sage: E(6).galois_conjugates(5)
File "/release/merger/sage-5.7.beta3/local/lib/python/site-packages/sage/rings/universal_cyclotomic_field/universal_cyclotomic_field
.py", line 1765, in galois_conjugates
raise ValueError("The given integer (%s) is not a multiple of the field order of %s"%(m,self))
ValueError: The given integer (5) is not a multiple of the field order of -E(3)^2
**********************************************************************
</pre>
Ticketstumpc5Fri, 01 Feb 2013 08:29:46 GMTattachment set
https://trac.sagemath.org/ticket/8327
https://trac.sagemath.org/ticket/8327
<ul>
<li><strong>attachment</strong>
set to <em>trac_8327-universal_cyclotomic_field-cs.patch</em>
</li>
</ul>
Ticketstumpc5Fri, 01 Feb 2013 08:33:33 GMT
https://trac.sagemath.org/ticket/8327#comment:203
https://trac.sagemath.org/ticket/8327#comment:203
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:202" title="Comment 202">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
Now that you have changed the exceptions, also the doctests for the exceptions need to changed.
</p>
</blockquote>
<p>
Sorry, I exported the patch before qrefreshing...
</p>
TicketchapotonFri, 01 Feb 2013 09:10:57 GMTstatus changed
https://trac.sagemath.org/ticket/8327#comment:204
https://trac.sagemath.org/ticket/8327#comment:204
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
TicketchapotonFri, 01 Feb 2013 09:51:48 GMTstatus changed
https://trac.sagemath.org/ticket/8327#comment:205
https://trac.sagemath.org/ticket/8327#comment:205
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
ok, bot is happy again. Positive review once more.
</p>
TicketjdemeyerTue, 05 Feb 2013 08:16:37 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/8327#comment:206
https://trac.sagemath.org/ticket/8327#comment:206
<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.7.beta3</em>
</li>
</ul>
TicketnthieryTue, 05 Feb 2013 17:11:45 GMT
https://trac.sagemath.org/ticket/8327#comment:207
https://trac.sagemath.org/ticket/8327#comment:207
<p>
Yippee! Congratulations Christian!
</p>
<p>
And thanks so much for going the extra mile! I agree that all the extra work you did for getting this ticket into Sage involved not only necessary cleanup but also extra features that could have been left aside in a first round!
</p>
<p>
Thanks!
</p>
<blockquote>
<p>
Nicolas
</p>
</blockquote>
Ticketstumpc5Wed, 06 Feb 2013 09:36:38 GMT
https://trac.sagemath.org/ticket/8327#comment:208
https://trac.sagemath.org/ticket/8327#comment:208
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/8327#comment:207" title="Comment 207">nthiery</a>:
</p>
<blockquote class="citation">
<p>
Yippee!
</p>
</blockquote>
<p>
+1!!!
</p>
TicketkcrismanFri, 15 Feb 2013 03:26:01 GMT
https://trac.sagemath.org/ticket/8327#comment:209
https://trac.sagemath.org/ticket/8327#comment:209
<p>
Just fyi, <a class="closed ticket" href="https://trac.sagemath.org/ticket/14118" title="defect: Compiling universal_cyclotomic_field_c.pyx fails on Cygwin (closed: fixed)">#14118</a> had to be opened regarding this, not that Cygwin is currently supported (yet!). Hopefully this will make it more portable in the long run anyway.
</p>
Ticket