Opened 10 years ago

Implement the universal cyclotomic field, using Zumbroich basis — at Version 147

Reported by: Owned by: nthiery major sage-5.7 number fields Cyclotomic field, Zumbroich basis sage-combinat, cwitty Christian Stump, Simon King N/A #13727, #13728

This patch provides the universal cyclotomic field

```    sage: UCF.<E> = UniversalCyclotomicField(); UCF
Universal Cyclotomic Field endowed with the Zumbroich basis
```

in sage. This field is the smallest field extension of QQ which contains all roots of unity.

```    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
```

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)k.

```    sage: UCF.zumbroich_basis(6)
[(6, 2), (6, 4)]
```

As seen for E(6), every element in UCF is expressed in terms of the smallest cyclotomic field in which it is contained.

```sage: E(6)*E(4)
-E(12)^11
```

It provides arithmetics on UCF as addition, multiplication, and inverses:

```    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
```

And also things like Galois conjugates.

```    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]
```

The ticket does not use the gap interface.

comment:2 follow-up: ↓ 3 Changed 10 years ago by craigcitro

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.

However, I'd like to suggest we might want to use a different convention for the constructor -- I don't like the idea of `CyclotomicField()` working. That seems like something that's too easy for a user to accidentally do (leave out the `n` 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 `F = CyclotomicField(n=Infinity)`?

comment:3 in reply to: ↑ 2 ; follow-up: ↓ 4 Changed 10 years ago by nthiery

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.

However, I'd like to suggest we might want to use a different convention for the constructor -- I don't like the idea of `CyclotomicField()` working. That seems like something that's too easy for a user to accidentally do (leave out the `n` 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 `F = CyclotomicField(n=Infinity)`?

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.

comment:4 in reply to: ↑ 3 ; follow-up: ↓ 5 Changed 10 years ago by craigcitro

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.

True. Maybe it's a bit too "cutesy," but using `n=0` might be nice -- after all, the `n`th cyclotomic field has roots of unity for all divisors of `n`, so this would still hold for the universal cyclotomic field and `n=0`.

comment:5 in reply to: ↑ 4 Changed 10 years ago by nthiery

True. Maybe it's a bit too "cutesy," but using `n=0` might be nice -- after all, the `n`th cyclotomic field has roots of unity for all divisors of `n`, so this would still hold for the universal cyclotomic field and `n=0`.

Mmm, any complex number is a 0-th root of unity, isn't it?

comment:6 Changed 10 years ago by stumpc5

• Status changed from new to needs_work

Does anyone have a pdf copy of Zumbroich's thesis where the algorithms are described?

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.

comment:7 Changed 10 years ago by stumpc5

The attached class NewCyclotomicField? uses the modified Zumbroich basis and behaves already somehow as it should (beside pretty printing).

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).

Here are some implementation problems I have:

• how can I replace CombinatorialAlgebra? by a new field constructor (I haven't found a description how to define a field)?

comment:9 Changed 10 years ago by stumpc5

• Status changed from needs_work to needs_review

comment:10 Changed 10 years ago by stumpc5

• Authors set to Christian Stump

comment:11 Changed 10 years ago by stumpc5

• Description modified (diff)

comment:12 Changed 10 years ago by davidloeffler

• Milestone set to sage-feature
• Status changed from needs_review to needs_work

Apply trac_8327_universal_cyclotomic_field-cs.2.patch

(for patchbot -- I hope that's right!).

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 `"raise NotImplementedError"`). Also, the new module should be added to the reference manual, and the formatting should be valid ReST markup.

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.

comment:13 follow-up: ↓ 14 Changed 10 years ago by davidloeffler

• Work issues set to Won't build, needs more doctests and Sphinxified docstrings

I've looked at the patchbot logs, and it gets worse -- the Cython code isn't valid apparently.

comment:14 in reply to: ↑ 13 Changed 10 years ago by stumpc5

I've looked at the patchbot logs, and it gets worse -- the Cython code isn't valid apparently.

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.

I improved the documentation but there are still some things to be done...

comment:15 Changed 9 years ago by stumpc5

• Status changed from needs_work to needs_review

For the buildbot:

Apply trac_8327_universal_cyclotomic_field-cs.patch

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.

Would be nice to get some feedback!

comment:16 Changed 9 years ago by stumpc5

Can some delete all but the youngest attached patch, the buildbot gets confused.

Thanks, Christian

comment:17 Changed 9 years ago by stumpc5

• Description modified (diff)

comment:18 Changed 9 years ago by davidloeffler

Apply trac_8327_universal_cyclotomic_field-cs.patch

comment:19 Changed 9 years ago by stumpc5

Apply only trac_8327_universal_cyclotomic_field-cs.patch

comment:20 Changed 9 years ago by stumpc5

• Work issues changed from Won't build, needs more doctests and Sphinxified docstrings to needs more doctests and Sphinxified docstrings

Apply only trac_8327_universal_cyclotomic_field-cs.patch

comment:21 Changed 9 years ago by stumpc5

Apply only trac_8327_universal_cyclotomic_field-cs.patch

comment:22 Changed 9 years ago by chapoton

Apply trac_8327_universal_cyclotomic_field-cs.patch

comment:23 Changed 9 years ago by stumpc5

• Dependencies set to #10963
• Owner changed from davidloeffler to (none)

comment:24 Changed 9 years ago by stumpc5

• Milestone changed from sage-feature to sage-4.7.2

comment:25 Changed 9 years ago by stumpc5

• Dependencies #10963 deleted

comment:26 follow-up: ↓ 27 Changed 9 years ago by nthiery

• Status changed from needs_review to needs_work

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 (#11761)

comment:27 in reply to: ↑ 26 Changed 9 years ago by SimonKing

• Authors changed from Christian Stump to Christian Stump, Simon King
• Status changed from needs_work to needs_review

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 (#11761)

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.

Note that the my patch does not add the new Cython as a dependency - it should run with the old Cython as well.

However, I already get a doctest error in `devel/sage-combinat/doc/en/thematic_tutorials/sandpile.rst`. 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.

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".

Apply trac_8327_universal_cyclotomic_field-cs.patch trac_8327_universal_cyclotomic_field_new_cython-sk.patch

comment:28 Changed 9 years ago by SimonKing

• Description modified (diff)

Sorry, I forgot to change the ticket description.

comment:29 Changed 9 years ago by SimonKing

• Status changed from needs_review to needs_work
• Work issues changed from needs more doctests and Sphinxified docstrings to needs more doctests and Sphinxified docstrings, rebase first patch

Note that the first patch does not apply on a clean sage-4.8.alpha0: The hunk that adds `from sage.misc.lazy_import import lazy_import` to sage/categories/all.py does not match.

But is importing lazy_import really needed? If I see that correctly, it is not used in sage/categories/all.py

comment:30 follow-up: ↓ 31 Changed 9 years ago by SimonKing

• Work issues changed from needs more doctests and Sphinxified docstrings, rebase first patch to Find dependency. Needs more doctests and Sphinxified docstrings.

Gosh. And my patch fails to apply as well, in both hunks.

So, apparently there is a dependency in the combinat queue. It should be named as a dependency for this ticket.

comment:31 in reply to: ↑ 30 Changed 9 years ago by SimonKing

• Work issues changed from Find dependency. Needs more doctests and Sphinxified docstrings. to Find dependency or rebase first patch. Needs more doctests and Sphinxified docstrings.

Gosh. And my patch fails to apply as well, in both hunks.

O dear, where is my mind? I had accidentally reverted the order of the patches when I tried to apply my patch.

So, for the record: The first patch does need to be rebased (or a dependency must be given). The second patch applies cleanly.

comment:32 Changed 9 years ago by SimonKing

• Work issues changed from Find dependency or rebase first patch. Needs more doctests and Sphinxified docstrings. to Find dependency or rebase first patch. Fix doctests. Needs more doctests and Sphinxified docstrings.

There are several doctest errors, even when one just has

```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
```

with the obvious modification on `trac_8327_universal_cyclotomic_field-cs.patch`.

Namely:

```        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
```

This one here

```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]
```

could be caused by my patch, which touches `ZumbroichDecomposition`. However, I have no idea about the other errors.

comment:33 follow-up: ↓ 34 Changed 9 years ago by stumpc5

Thanks for working in this patch -- I will have a look at it this morning and see what the issues are...

Best, Christian

comment:34 in reply to: ↑ 33 ; follow-up: ↓ 35 Changed 9 years ago by stumpc5

Thanks for working in this patch -- I will have a look at it this morning and see what the issues are...

I guess, I should first compile 4.7.2; that will take a while first...

comment:35 in reply to: ↑ 34 Changed 9 years ago by nthiery

Thanks for working in this patch -- I will have a look at it this morning and see what the issues are...

Cool!

I guess, I should first compile 4.7.2; that will take a while first...

4_8 alpha0 actually

Cheers,

comment:36 follow-up: ↓ 37 Changed 9 years ago by stumpc5

• Description modified (diff)
• Status changed from needs_work to needs_review

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).

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...

comment:37 in reply to: ↑ 36 ; follow-up: ↓ 38 Changed 9 years ago by SimonKing

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).

Good! I'll test it a bit later.

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?

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.

comment:38 in reply to: ↑ 37 ; follow-up: ↓ 39 Changed 9 years ago by davidloeffler

• Work issues changed from Find dependency or rebase first patch. Fix doctests. Needs more doctests and Sphinxified docstrings. to Find dependency or rebase first patch. Fix doctests.

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.

(That was 10 months ago and applied to a much earlier version of the patch.)

comment:39 in reply to: ↑ 38 Changed 9 years ago by stumpc5

(That was 10 months ago and applied to a much earlier version of the patch.)

I now remember seeing it back then, the patch indeed changed substantially since then. So I guess it simply needs a new review.

comment:40 Changed 9 years ago by davidloeffler

It's perfectly permissible, and indeed a good idea, to clear the "work issues" field when uploading a new patch.

comment:41 follow-ups: ↓ 42 ↓ 44 Changed 9 years ago by SimonKing

• Dependencies set to #10771
• Status changed from needs_review to needs_work
• Work issues changed from Find dependency or rebase first patch. Fix doctests. to Rebase wrt #10771. Fix doctests.

When starting with sage-4.8.alpha0 plus the stuff from #11761 (new cython), one quite big hunk of your new patch fails to apply. It concerns sage/categories/fields.py.

I found at least one reason for the mismatch: By trac ticket #10771 (sorry, it's one of mine...), `Fields()` has some `ElementMethods` (your patch assumes that there are none). Since #10771 has been merged in sage-4.7.2, I make this a dependency.

Well, I understand that you are building 4.7.2 anyway, aren't you?

comment:42 in reply to: ↑ 41 ; follow-up: ↓ 43 Changed 9 years ago by stumpc5

When starting with sage-4.8.alpha0 plus the stuff from #11761 (new cython), one quite big hunk of your new patch fails to apply. It concerns sage/categories/fields.py.

I found at least one reason for the mismatch: By trac ticket #10771 (sorry, it's one of mine...), `Fields()` has some `ElementMethods` (your patch assumes that there are none). Since #10771 has been merged in sage-4.7.2, I make this a dependency.

Well, I understand that you are building 4.7.2 anyway, aren't you

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)

comment:43 in reply to: ↑ 42 Changed 9 years ago by SimonKing

I am building 4.7.2 and 4.8, but te later just failed to build...

What, 4.8.alpha0 (or alpha1) failed to build?? That should not happen. Perhaps you can report on sage-release?

comment:44 in reply to: ↑ 41 ; follow-up: ↓ 45 Changed 9 years ago by nthiery

When starting with sage-4.8.alpha0 plus the stuff from #11761 (new cython), one quite big hunk of your new patch fails to apply. It concerns sage/categories/fields.py.

I found at least one reason for the mismatch: By trac ticket #10771 (sorry, it's one of mine...), `Fields()` has some `ElementMethods` (your patch assumes that there are none). Since #10771 has been merged in sage-4.7.2, I make this a dependency.

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.

comment:45 in reply to: ↑ 44 Changed 9 years ago by stumpc5

When starting with sage-4.8.alpha0 plus the stuff from #11761 (new cython), one quite big hunk of your new patch fails to apply. It concerns sage/categories/fields.py.

I found at least one reason for the mismatch: By trac ticket #10771 (sorry, it's one of mine...), `Fields()` has some `ElementMethods` (your patch assumes that there are none). Since #10771 has been merged in sage-4.7.2, I make this a dependency.

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.

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...

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...).

comment:46 Changed 9 years ago by SimonKing

The new patch does not apply, because of the hunk

```--- all.py
+++ all.py
@@ -1,3 +1,5 @@
+from sage.misc.lazy_import import lazy_import
+
from category import    is_Category, Category, HomCategory

```

The point is that this hunk shold be totally removed, because lazy import is not needed in sage/categories/all.py.

For testing, I'm doing this change myself. But please post a patch without that hunk

comment:47 follow-up: ↓ 48 Changed 9 years ago by SimonKing

• Status changed from needs_work to needs_review

Sorry, lazy_import is used. So, don't delete the patch. However, it does not apply. Reason: The first line of sage/categories/all.py currently is

```from category import    is_Category, Category, HomCategory, AbstractCategory
```

My guess is that you worked on top of Nicolas's "multiple realizations" patch.

So, either submit a patch on trac that differs from the one in the combinat queue, or name #7980 as a dependency.

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.

comment:48 in reply to: ↑ 47 ; follow-up: ↓ 49 Changed 9 years ago by stumpc5

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.

I am a little lost now: you don't want the current patch but the one I had before without Nicolas' rebase?

comment:49 in reply to: ↑ 48 Changed 9 years ago by SimonKing

I am a little lost now: you don't want the current patch but the one I had before without Nicolas' rebase?

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 #7980.

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.

comment:50 Changed 9 years ago by SimonKing

I get one error in startup.py:

```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
```

Do you import numpy at startup? Nicolas has just found that ticket #11714 explicitly avoids numpy being loaded at startup.

comment:51 Changed 9 years ago by SimonKing

You import and cimport numpy. Nicolas suggest to try and lazy_import numpy instead of import (cimport should be fine).

comment:52 Changed 9 years ago by SimonKing

... or lazy-import the stuff from sage/rings/universal_cyclotomic_field/all.py? I'm preparing a patch that does this.

comment:53 Changed 9 years ago by SimonKing

• Description modified (diff)

OK, done.

When applying both patches, one gets

```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
```

So, numpy is only imported when needed, and this is what #11714 was about.

Apply trac_8327_universal_cyclotomic_field-cs.patch trac8327_lazy_import_UCF.patch

comment:54 follow-up: ↓ 55 Changed 9 years ago by SimonKing

• Status changed from needs_review to needs_work
• Work issues changed from Rebase wrt #10771. Fix doctests. to Rebase wrt #10771. Fix one doctest.

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:

```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 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]
```

Any idea why that occurs?

comment:55 in reply to: ↑ 54 ; follow-up: ↓ 56 Changed 9 years ago by stumpc5

sage: UCF.is_subring(UCF)

Expected:

True

Got:

False

UCF.is_subring(UCF) just returns

```sage: UCF is UCF
```

and as `UniversalCyclotomicField` inherits from `UniqueRepresentation`, this should return `True`. I don't know if this error might come from some lazy import which makes UCF not unique, or if something changed in `UniqueRepresentation`? Nicolas might know...

comment:56 in reply to: ↑ 55 ; follow-up: ↓ 57 Changed 9 years ago by nthiery

UCF.is_subring(UCF) just returns

```sage: UCF is UCF
```

and as `UniversalCyclotomicField` inherits from `UniqueRepresentation`, this should return `True`.

That's independent of UniqueRepresentation?: X is X is true for any Python object X.

So the test above breaks, my best bet is that the is_ring method that gets called is the wrong one.

Good chase!

Nicolas

comment:57 in reply to: ↑ 56 Changed 9 years ago by stumpc5

Replying to stumpc5: So the test above breaks, my best bet is that the is_ring method that gets called is the wrong one.

I found the problem with building sage (see #11970).

4.7.2 has finished, and I now build 4.8.alpha1. I will investigate the above problem as soon as it is finished...

comment:58 follow-up: ↓ 59 Changed 9 years ago by stumpc5

• Description modified (diff)
• Status changed from needs_work to needs_review

It is rebased and applies now on a new 4.8.alpha1 (NOT alpha0, but alpha1!)

There was a syntax error on the plain 4.8.alpha1, which I first had to get rid off.

Concerning the is_subring method, I get the following behaviour:

```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
```

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!

comment:59 in reply to: ↑ 58 ; follow-up: ↓ 60 Changed 9 years ago by nthiery

```sage: UCF.is_subring(UCF)
False
```

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!

Ach ja! Sorry, I should have thought about this: it is an instance of #10906: "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:

```from sage.rings.universal_cyclotomic_field.universal_cyclotomic_field_c import *
```

by explicit lazy imports of the various functions:

```lazy_import('sage.rings.universal_cyclotomic_field.universal_cyclotomic_field_c', 'ZumbroichBasis...')
```

or maybe just lazy import that module as, say, ucfc, and qualify all its functions with it.

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!

Cheers,

Nicolas

comment:60 in reply to: ↑ 59 ; follow-up: ↓ 61 Changed 9 years ago by stumpc5

```lazy_import('sage.rings.universal_cyclotomic_field.universal_cyclotomic_field_c', 'ZumbroichBasis...')
```

I now do that, and all tests pass -- but I somehow missed why we want to lazy import anything; is it about importing numpy?

comment:61 in reply to: ↑ 60 ; follow-up: ↓ 62 Changed 9 years ago by SimonKing

I now do that, and all tests pass -- but I somehow missed why we want to lazy import anything; is it about importing numpy?

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 really need it.

comment:62 in reply to: ↑ 61 ; follow-up: ↓ 63 Changed 9 years ago by stumpc5

Replying to stumpc5: 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 really need it.

I see -- I made some mistake last night, but now, the cython part is lazy imported, and all tests pass...

comment:63 in reply to: ↑ 62 Changed 9 years ago by nthiery

I see -- I made some mistake last night, but now, the cython part is lazy imported, and all tests pass...

Cool. Thanks! This is as much saved for Sage's startup.

Cheers,

Nicolas

comment:64 follow-up: ↓ 65 Changed 9 years ago by mhansen

Ping. I think we should try to get this in shortly.

comment:65 in reply to: ↑ 64 Changed 9 years ago by stumpc5

Ping. I think we should try to get this in shortly.

+1.

Simon, are you planning to review it? Or anyone else volunteering?

Christian

comment:66 follow-up: ↓ 67 Changed 9 years ago by SimonKing

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

Similarly, where is trac8327_lazy_import_UCF.patch ?

I thought it is impossible (unless for administrators) to remove an attachment from trac?

Also, there are two work issues mentioned. Are these fixed? I.e., is it really "needs review", or is it "needs work"?

comment:67 in reply to: ↑ 66 ; follow-ups: ↓ 68 ↓ 69 Changed 9 years ago by stumpc5

• Work issues Rebase wrt #10771. Fix one doctest. deleted

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

Similarly, where is trac8327_lazy_import_UCF.patch ?

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).

I thought it is impossible (unless for administrators) to remove an attachment from trac?

I guess, I have admin rights then.

Also, there are two work issues mentioned. Are these fixed? I.e., is it really "needs review", or is it "needs work"?

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).

Best, Christian

comment:68 in reply to: ↑ 67 Changed 9 years ago by davidloeffler

(though I don't know if there is another rebase needed for sage-4.8.alpha4).

No need -- patch applies fine to 4.8.alpha4. The new files pass their doctests (I haven't run a full doctest cycle).

comment:69 in reply to: ↑ 67 ; follow-up: ↓ 70 Changed 9 years ago by SimonKing

I thought it is impossible (unless for administrators) to remove an attachment from trac?

I guess, I have admin rights then.

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:

Apply trac_8327_universal_cyclotomic_field-cs.patch

But then, what is the status of your "syntax error" patch?

comment:70 in reply to: ↑ 69 Changed 9 years ago by stumpc5

But then, what is the status of your "syntax error" patch?

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.

comment:71 Changed 8 years ago by davidloeffler

• Dependencies changed from #10771 to #4539 #10771
• Description modified (diff)

Apply trac_8327-rebased_for_v5.patch

(for the patchbot).

The previous patch conflicts with #4539, so I have done a new rebased patch. There was one change in `free_module.py` 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.

comment:72 Changed 8 years ago by davidloeffler

• Status changed from needs_review to needs_work

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 #7980.

comment:73 Changed 8 years ago by davidloeffler

• Status changed from needs_work to needs_review

I've re-rebased the patch.

comment:74 Changed 8 years ago by davidloeffler

Apply trac_8327-rebased_for_v5.patch, trac_8327-docfix.patch

(I got some error messages building the documentation -- the above patch fixes them.)

comment:75 follow-up: ↓ 78 Changed 8 years ago by davidloeffler

• Authors changed from Christian Stump, Simon King to Christian Stump, Simon King, David Loeffler
• Description modified (diff)

comment:76 Changed 8 years ago by davidloeffler

• Dependencies changed from #4539 #10771 to #4539 #10771 #7980

comment:77 Changed 8 years ago by davidloeffler

Apply trac_8327-rebased_for_v5.patch, trac_8327-docfix.patch

(patchbot's confused again)

comment:78 in reply to: ↑ 75 Changed 8 years ago by nthiery

Hi guys,

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?

Cheers,

Nicolas

comment:79 Changed 8 years ago by davidloeffler

• Authors changed from Christian Stump, Simon King, David Loeffler to Christian Stump, Simon King
• Reviewers set to David Loeffler

Sure -- my contribution was just rebasing + reformatting some docstrings. I've removed myself as author (but added myself as reviewer).

comment:80 Changed 8 years ago by stumpc5

Hi,

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)!

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

+ all my new docstrings

+ 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.)

+ David's improvements in the docstrings

?

Best, Christian

comment:81 follow-up: ↓ 82 Changed 8 years ago by davidloeffler

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 #7980, which was only merged in beta11. So please build a copy of beta11 before you do any further work on this.

comment:82 in reply to: ↑ 81 ; follow-up: ↓ 83 Changed 8 years ago by stumpc5

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 #7980, which was only merged in beta11. So please build a copy of beta11 before you do any further work on this.

Alright, I will then first build and do attach another version tomorrow then.

comment:83 in reply to: ↑ 82 Changed 8 years ago by nthiery

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 #7980, which was only merged in beta11. So please build a copy of beta11 before you do any further work on this.

Alright, I will then first build and do attach another version tomorrow then.

For the record: if Christian's latest version is that on the sage-combinat patch server, then it should be already based on #7980 and likely to apply smoothly.

comment:84 Changed 8 years ago by stumpc5

• Description modified (diff)

comment:85 Changed 8 years ago by stumpc5

I attached a new version that contains everything I have, which is rebased for 5.0.beta11, and also contains David's docfixes.

comment:86 Changed 8 years ago by davidloeffler

Apply trac_8327_universal_cyclotomic_field-cs.patch

(for patchbot)

comment:87 follow-up: ↓ 88 Changed 8 years ago by davidloeffler

• Reviewers David Loeffler deleted
• Status changed from needs_review to needs_work
```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 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 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
**********************************************************************
```

(All three of these failures had already been reported, and fixed, before this latest patch re-broke them.)

comment:88 in reply to: ↑ 87 ; follow-up: ↓ 89 Changed 8 years ago by stumpc5

(All three of these failures had already been reported, and fixed, before this latest patch re-broke them.)

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.)

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?

comment:89 in reply to: ↑ 88 ; follow-up: ↓ 90 Changed 8 years ago by davidloeffler

• Work issues set to don't import numpy

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?

It is related to your patch, as discussed in the comment thread above. It is important that Sage must not import numpy at startup, because it takes a very long time to load (cf. tickets #11714, #6494, 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 `lazy_import` to stop this happening (see comments 50-53). You will see that trac_8327-rebased_for_v5.patch does not need to disable this test in startup.py as you have done.

comment:90 in reply to: ↑ 89 Changed 8 years ago by stumpc5

It is related to your patch, as discussed in the comment thread above. It is important that Sage must not import numpy at startup, because it takes a very long time to load (cf. tickets #11714, #6494, 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 `lazy_import` to stop this happening (see comments 50-53). You will see that trac_8327-rebased_for_v5.patch does not need to disable this test in startup.py as you have done.

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.

comment:91 Changed 8 years ago by stumpc5

• Status changed from needs_work to needs_review
• Work issues don't import numpy deleted

numpy is now lazy imported:

```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
```

I hope I haven't forgotten any further things!

comment:92 Changed 8 years ago by davidloeffler

Apply trac_8327_universal_cyclotomic_field-cs.patch

(for patchbot)

comment:93 follow-up: ↓ 94 Changed 8 years ago by davidloeffler

• Status changed from needs_review to needs_work

Right, so now we have a patch which actually works, we can start testing it. And we find the following bug:

```----------------------------------------------------------------------
| 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>()

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)()

```

This is even worse, since the element involved isn't cyclotomic in the first place:

```sage: K.<a> = NumberField(x^3 - 7)
sage: UCF(a)
a
sage: UCF(a) in UCF
True
```

In fact UCF will attempt to construct an element from anything whatsoever:

```sage: UCF(None)
None
sage: UCF(QQ)
Rational Field
sage: UCF('hello')
'hello'
```

The following are not great either:

```sage: UCF.has_coerce_map_from(CyclotomicField(3))
False
sage: QQbar.has_coerce_map_from(UCF)
False
```

comment:94 in reply to: ↑ 93 ; follow-up: ↓ 95 Changed 8 years ago by stumpc5

In fact UCF will attempt to construct an element from anything whatsoever:

```sage: UCF(None)
None
sage: UCF(QQ)
Rational Field
sage: UCF('hello')
'hello'
```

I didn't implement the `__call__` method - what is the behaviour you would expect here?

The following are not great either:

```sage: UCF.has_coerce_map_from(CyclotomicField(3))
False
```

how can I get a coerce from `CyclotomicField(n)` for all `n`?

```sage: QQbar.has_coerce_map_from(UCF)
False
```

I fixed this.

comment:95 in reply to: ↑ 94 ; follow-up: ↓ 96 Changed 8 years ago by davidloeffler

I didn't implement the `__call__` method - what is the behaviour you would expect here?

Oh, I don't know, maybe the same behaviour as every other parent object in the Sage library? The default ` __call__ ` calls ` _element_constructor_ `, 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.

You should also set the attribute wrapped_class for your element class to something other than the default (which is "object"), so the ` __init__` it inherits from ElementWrapper knows what it's supposed to be wrapping and doesn't wrap obviously bogus objects as in the examples above.

how can I get a coerce from `CyclotomicField(n)` for all `n`?

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 ` isinstance(K, sage.rings.number_field.number_field.NumberField_cyclotomic) ` or perhaps ` sage.rings.number_field.number_field.is_CyclotomicField(K) ` helpful. You should also check ` K.coerce_embedding`, since it's possible to create cyclotomic fields with complex embeddings other than the standard one.

comment:96 in reply to: ↑ 95 Changed 8 years ago by stumpc5

Replying to stumpc5: Oh, I don't know, maybe the same behaviour as every other parent object in the Sage library? The default ` __call__ ` calls ` _element_constructor_ `, 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.

The idea is that one should *not* construct elements of the universal cyclotomic field using `UCF(arg)` but only through the function `E`, e.g., `E(5)`. So `UCF(arg)` should only be called for coercion.

I could overwrite `_element_constructor_` by

```def _element_constructor_(self,arg):
raise TypeError, "No coercion found for %s"%str(arg)
```

This would prevent `UCF` from ever constructing any element, and only using the coercion model to coerce objects if possible.

You should also set the attribute wrapped_class for your element class to something other than the default (which is "object"), so the ` __init__` it inherits from ElementWrapper knows what it's supposed to be wrapping and doesn't wrap obviously bogus objects as in the examples above.

Do I see it right that this would then also be obsolete?

how can I get a coerce from `CyclotomicField(n)` for all `n`?

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?

Let me define one coercion:

```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)
```

First, how can I register this coercion for all `n`? Second, are there further number fields I should have a coercion from?

comment:97 follow-up: ↓ 98 Changed 8 years ago by davidloeffler

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.

I think I've had enough of this ticket; perhaps someone else can take over "reviewing" it from here onwards.

comment:98 in reply to: ↑ 97 Changed 8 years ago by stumpc5

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.

I think I've had enough of this ticket; perhaps someone else can take over "reviewing" it from here onwards.

First, I consider this strongly unfriendly.

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.

comment:99 follow-up: ↓ 100 Changed 8 years ago by davidloeffler

I'm sorry, please pardon my earlier outburst -- it was uncalled for, your message just caught me at a bad time.

See http://www.sagemath.org/doc/reference/coercion.html#methods-to-implement. The magic routines here are ` _element_constructor_` and ` _coerce_map_from_ `. In this case, you can make ` UCF._coerce_map_from_(K) ` 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).

Last edited 8 years ago by davidloeffler (previous) (diff)

comment:100 in reply to: ↑ 99 Changed 8 years ago by stumpc5

See http://www.sagemath.org/doc/reference/coercion.html#methods-to-implement. The magic routines here are

` _coerce_map_from_ `

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 `UCF`.

` _element_constructor_`

I want that an element of `UCF` is never constructed using the `__call__` method but only using the function `E`. In the 4 steps in http://www.sagemath.org/doc/reference/coercion.html#provided-methods `__call__`, I want the first 3 to proceed, and if no coercion is found, that an error is raised. The idea here is that `UCF` has an infinite number of generators, and the easiest to play with them seems to be the same way this is done in `GAP`; simply doing things like

```sage: E(3)
E(3)
sage: E(3)+E(4)
E(12)^4 - E(12)^7 - E(12)^11
```

Also, I want to see

```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
```

Is this generally wrong, or is the way I proposed to define `_element_constructor_` in my last comment just not the way to do that?

comment:101 follow-up: ↓ 102 Changed 8 years ago by chapoton

Ok, I am willing to try a review. I am not sure to be competent enough. We will see.

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 ?

comment:102 in reply to: ↑ 101 Changed 8 years ago by stumpc5

Ok, I am willing to try a review. I am not sure to be competent enough. We will see.

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 ?

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.

comment:103 follow-up: ↓ 104 Changed 8 years ago by stumpc5

• Dependencies changed from #4539 #10771 #7980 to #4539, #10771, #7980, #13727, #13728

comment:104 in reply to: ↑ 103 Changed 8 years ago by stumpc5

For a little better readability, I started by extracting two minor changes in other parts of Sage into two new tickets, #13727 and #13728.

comment:105 Changed 8 years ago by stumpc5

• Dependencies changed from #4539, #10771, #7980, #13727, #13728 to #4539, #10771, #7980, #9651, #13727, #13728
• Description modified (diff)
• Status changed from needs_work to needs_review

comment:106 Changed 8 years ago by stumpc5

• Dependencies changed from #4539, #10771, #7980, #9651, #13727, #13728 to #13727, #13728

comment:107 follow-up: ↓ 108 Changed 8 years ago by stumpc5

I uploaded a new version of the patch - let's see what the patchbot says.

@Fred: when you start reviewing the patch, please don't try to understand what happens in `sage/rings/universal_cyclotomic_field/universal_cyclotomic_field_c.pyx`. 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.

comment:108 in reply to: ↑ 107 Changed 8 years ago by stumpc5

I uploaded a new version of the patch - let's see what the patchbot says.

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?

comment:109 Changed 8 years ago by SimonKing

Really strange. For the record: The patchbot says

```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
```

but if you search the string ".rst" in attachment:trac_8327_universal_cyclotomic_field-cs.patch, you only find that `doc/en/reference/rings_standard.rst` is touched.

comment:110 follow-up: ↓ 111 Changed 8 years ago by chapoton

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.

comment:111 in reply to: ↑ 110 ; follow-up: ↓ 112 Changed 8 years ago by stumpc5

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.

I now divides the patch in three parts, only the last touching the documentation...

comment:112 in reply to: ↑ 111 ; follow-up: ↓ 114 Changed 8 years ago by stumpc5

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.

I now divides the patch in three parts, only the last touching the documentation...

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).

comment:113 Changed 8 years ago by stumpc5

• Dependencies changed from #13727, #13728 to #13727, #13728, #13734, #13735

comment:114 in reply to: ↑ 112 Changed 8 years ago by stumpc5

it seems to work now

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).

Beside that, I added 100% coverage everywhere, and excluded 4 very small side tickets to be reviewed directly.

Cheers, Christian

comment:115 follow-ups: ↓ 116 ↓ 120 Changed 8 years ago by chapoton

• why are we changing the import of hecke_modules in this patch ?
• the doc of prime_subfield has a mistake : ZZ should be QQ
• in from_dict, only one parameter is documented
• the docs of _eq_ and _ne_ are strange, and look like a copy and paste
• is_unit should be removed, as already implemented in the class of Field elements
• it would be worth adding some of the methods of QQbar elements : minpoly and abs maybe
• maybe consider a coercion of real elements to the field AA ?
• somewhere near the beginning of the doc, it should be explained the following important points

1 the field UCF is contained in the complex field

2 how to coerce z to QQbar : QQbar(z)

3 how to coerce z to the minimal cyclotomic field containing it : z.to_cyclotomic_field()

That's all for today.

comment:116 in reply to: ↑ 115 ; follow-up: ↓ 117 Changed 8 years ago by stumpc5

• Dependencies changed from #13727, #13728, #13734, #13735 to #13727, #13728

• 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).
• I took your remarks into account:
• why are we changing the import of hecke_modules in this patch ?

because the import of hecke_modules and of universal_cyclotomic_field causes an import loop. Since we want to use `UCF`, it was easier and cleaner to lazily import hecke_modules.

• is_unit should be removed, as already implemented in the class of Field elements

the inherited `is_unit` method is not capturing the situation. It's implementation is

```    def is_unit(self):
if self == 1 or self == -1:
return True
raise NotImplementedError
```
• it would be worth adding some of the methods of QQbar elements : minpoly and abs maybe

not yet done!

• maybe consider a coercion of real elements to the field AA ?

This is impossible, isn't it? Here is a quote from the documentation:

"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."

Or am I misunderstanding something?

1 the field UCF is contained in the complex field

This coercion is going through QQbar, so I only documented the later.

Best, Christian

comment:117 in reply to: ↑ 116 ; follow-up: ↓ 118 Changed 8 years ago by chapoton

• is_unit should be removed, as already implemented in the class of Field elements

the inherited `is_unit` method is not capturing the situation. It's implementation is

```    def is_unit(self):
if self == 1 or self == -1:
return True
raise NotImplementedError
```

I do not understand : if one look at Ticket #13728, the method is_unit does check that an element is not zero !

• maybe consider a coercion of real elements to the field AA ?

This is impossible, isn't it? Here is a quote from the documentation:

"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."

Or am I misunderstanding something?

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..

```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
```

1 the field UCF is contained in the complex field

This coercion is going through QQbar, so I only documented the later.

Yes, But I rather meant the mathematical point : the field is defined as an embedded field, not as an abstract field..

comment:118 in reply to: ↑ 117 Changed 8 years ago by stumpc5

I do not understand : if one look at Ticket #13728, the method is_unit does check that an element is not zero !

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?

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..

(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.

```sage: AA(QQbar(E(5) + E(5)^4))
0.618033988749895?
```

works, but one currently has to pass through QQbar.

Yes, But I rather meant the mathematical point : the field is defined as an embedded field, not as an abstract field..

Okay, I will add a sentence in the beginning.

Last edited 8 years ago by stumpc5 (previous) (diff)

comment:119 Changed 8 years ago by nthiery

Thanks so much Frédéric for taking over the review! (and in general for all your reviewing work!)

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.

comment:120 in reply to: ↑ 115 Changed 8 years ago by stumpc5

• it would be worth adding some of the methods of QQbar elements : minpoly and abs maybe

I added implementations of those, and I also improved the documentation here and there a bit.

comment:121 follow-ups: ↓ 122 ↓ 124 Changed 8 years ago by chapoton

• this import is apparently not used :

from sage.rings.complex_field import ComplexField?

• the sort of elements is backwards compared to the order for cyclotomic fields
```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
```

maybe it would be better to go the same way ? but maybe this is the same as gap ?

• Is the is_subring method necessary ? it is almost empty !
• in the element constructor, maybe one could answer by something like

"no coercion found for elements of XXX"

with XXX the parent of the argument ? It would be more informative, no ?

• 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..
• why is the hash function test tagged with random ? does it depend on the computer or something like that ?
• more generally, maybe one could try to avoid using assert, and better raise precise Exceptions ?
• I have not read seriously the cython part, but found a typo :

inverse for the univeral

That's all for today

comment:122 in reply to: ↑ 121 ; follow-up: ↓ 123 Changed 8 years ago by stumpc5

Thanks a lot!

• the sort of elements is backwards compared to the order for cyclotomic fields

maybe it would be better to go the same way ? but maybe this is the same as gap ?

I used the same order as gap (the increasing ordering on the exponents). I think I would prefer this ordering...

• Is the is_subring method necessary ? it is almost empty !

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?

"no coercion found for elements of XXX"

done, I had to distinguish between objects having a parent and those not having one.

• 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..

done

• why is the hash function test tagged with random ? does it depend on the computer or something like that ?

If I am not mistaken, the hash is only unique in a given Sage session.

• in _invert_, one should rather raise a ZeroDivisionError? when trying to invert 0 ?
• more generally, maybe one could try to avoid using assert, and better raise precise Exceptions ?

good point, done!

Best, Christian

comment:123 in reply to: ↑ 122 ; follow-up: ↓ 125 Changed 8 years ago by nthiery

• why is the hash function test tagged with random ? does it depend on the computer or something like that ?

If I am not mistaken, the hash is only unique in a given Sage session.

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:

sage: e = RootSystem?(['A',2]).ambient_space() sage: hash(e.simple_root(0)) -4601450286177489034 # 64-bit 549810038 # 32-bit

Cheers,

Nicolas

comment:124 in reply to: ↑ 121 Changed 8 years ago by nthiery

• more generally, maybe one could try to avoid using assert, and better raise precise Exceptions ?

Just a side note: as discussed on sage-combinat-devel, assertions are preferable in the following situations:

• For checking the internal state of the program
• When the caller should *not* catch the exception
• In time critical locations

Beside, nothing prevents from writing a meaningful message in an assertion check.

Cheers,

Nicolas

comment:125 in reply to: ↑ 123 ; follow-up: ↓ 126 Changed 8 years ago by stumpc5

• why is the hash function test tagged with random ? does it depend on the computer or something like that ?

If I am not mistaken, the hash is only unique in a given Sage session.

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

Thanks for the clarification!

Do you also happen to know how to handle the real conversion as in `AA(QQbar(E(5) + E(5)^4))` in comment:118 ?

comment:126 in reply to: ↑ 125 ; follow-up: ↓ 127 Changed 8 years ago by nthiery

• why is the hash function test tagged with random ? does it depend on the computer or something like that ?

If I am not mistaken, the hash is only unique in a given Sage session.

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

Thanks for the clarification!

Do you also happen to know how to handle the real conversion as in `AA(QQbar(E(5) + E(5)^4))` in comment:118 ?

Did you try implementing the (partial) morphism from UCF to AA (using SetMorphism? and the category of SetsWithPartialMaps?), and register it as a conversion?

If yes, this could possibly be done during the initialization of UCF.

comment:127 in reply to: ↑ 126 ; follow-ups: ↓ 128 ↓ 130 Changed 8 years ago by stumpc5

Did you try implementing the (partial) morphism from UCF to AA (using SetMorphism? and the category of SetsWithPartialMaps?), and register it as a conversion?

Good call, it works now! Thanks!

Btw: It would be nice to include this in the documentation, if not already done. At least if I google for `"sage SetMorphism SetsWithPartialMaps"`, I only get the file `root_space.py` which is perfectly fine to explain how to do it, but I only found it after you telling me what to look for.

comment:128 in reply to: ↑ 127 ; follow-up: ↓ 131 Changed 8 years ago by stumpc5

Good call, it works now! Thanks!

One more thing: do you also know how to do the same for the cyclotomic fields ? What I would need is something like

```        SetMorphism(
Hom(self, CyclotomicField(n), SetsWithPartialMaps()),
lambda elem: elem.to_cyclotomic_field()
).register_as_conversion()
```

The problem is that the parameter `n` is not determined a priori, and I want the conversion to exist for any `n`.

comment:129 Changed 8 years ago by chapoton

two minor suggestions :

• I would write "smallest subfield of the complex field" instead of "smallest subfield of the complex plane"
• 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'

This is great that you managed to coerce to AA !

comment:130 in reply to: ↑ 127 Changed 8 years ago by nthiery

I am glad that it worked out!

Btw: It would be nice to include this in the documentation, if not already done. At least if I google for `"sage SetMorphism SetsWithPartialMaps"`, I only get the file `root_space.py` which is perfectly fine to explain how to do it, but I only found it after you telling me what to look for.

Indeed! A good spot might be Simon's tutorial worksheet on coercion:

Simon?

comment:131 in reply to: ↑ 128 Changed 8 years ago by nthiery

One more thing: do you also know how to do the same for the cyclotomic fields ? What I would need is something like

```        SetMorphism(
Hom(self, CyclotomicField(n), SetsWithPartialMaps()),
lambda elem: elem.to_cyclotomic_field()
).register_as_conversion()
```

The problem is that the parameter `n` is not determined a priori, and I want the conversion to exist for any `n`.

There is no perfect support for this. But I guess you could change the initializtion of CyclotomicField? 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).

comment:132 follow-up: ↓ 134 Changed 8 years ago by stumpc5

Hi --

I think we are now some big steps forward!

• I took care of everything Fred was asking for, in particular the partial conversion to `AA`.

and

• 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.

Best, Christian

comment:133 follow-up: ↓ 135 Changed 8 years ago by chapoton

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..

comment:134 in reply to: ↑ 132 Changed 8 years ago by nthiery

Hi --

I think we are now some big steps forward!

• I took care of everything Fred was asking for, in particular the partial conversion to `AA`.

and

• 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.

Cool!

I guess it's fair if you only provide conversions to-from those cyclotomic field that use the default embedding.

comment:135 in reply to: ↑ 133 Changed 8 years ago by stumpc5

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..

I do agree, I didn't check that carefully. All I did was to add a 5 line method to `rings/number_field/number_field.py` that does the conversion from UCF to CF.

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?

comment:136 follow-up: ↓ 137 Changed 8 years ago by chapoton

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 ?

comment:137 in reply to: ↑ 136 Changed 8 years ago by stumpc5

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 ?

Do you accept it now as only a little change (which I actually consider a bit improvement), see my changes in `number_field.py` ?

comment:138 follow-up: ↓ 139 Changed 8 years ago by chapoton

yes, much better.

• there is still the typo "the univeral cyclotomic"

comment:139 in reply to: ↑ 138 ; follow-up: ↓ 140 Changed 8 years ago by stumpc5

yes, much better.

• there is still the typo "the univeral cyclotomic"

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.

It's fixed now and will be in the next version of the patch as soon as I have more things to change.

comment:140 in reply to: ↑ 139 Changed 8 years ago by stumpc5

It's fixed now and will be in the next version of the patch as soon as I have more things to change.

Okay, done -- I fixed this and added more documentation on getting to the cyclotomics.

comment:141 follow-up: ↓ 142 Changed 8 years ago by chapoton

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.

I think we can leave further improvements for later tickets. I have noted an error occuring when doing FractionField?(PolynomialRing?(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.

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.

One last thing : the following lines have to be modified, as this has been implemented :

.. TODO
modify CyclotomicField? to add a (partial) conversion from UCF

comment:142 in reply to: ↑ 141 Changed 8 years ago by stumpc5

Thanks a lot for doing the final review!

I don't see how my patch might have influenced the doc failure on `sage/schemes/elliptic_curves/ell_number_field.py`, its tests pass on my machine (in roughly 45 sec, independent of my patch being applied or not).

comment:143 follow-up: ↓ 146 Changed 8 years ago by chapoton

Well, I do indeed have this failing test :

sage -t -force_lib "devel/sage/sage/schemes/elliptic_curves/ell_number_field.py" Segmentation fault (core dumped)

with the patch applied, and it disappears when the patch is removed.

So I guess there is a problem somewhere..

comment:144 follow-up: ↓ 145 Changed 8 years ago by chapoton

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 ?

comment:145 in reply to: ↑ 144 Changed 8 years ago by nthiery

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..

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.

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 ?

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:

```    sage: UCF.inject_shorthands()
sage: E(3)
```

(as for symmetric functions)

comment:146 in reply to: ↑ 143 Changed 8 years ago by stumpc5

I just rechecked on my machine, and I get `All tests passed!` 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.