Opened 12 years ago
Last modified 9 years ago
#8327 closed enhancement
Implement the universal cyclotomic field, using Zumbroich basis — at Version 84
Reported by: | nthiery | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-5.7 |
Component: | number fields | Keywords: | Cyclotomic field, Zumbroich basis |
Cc: | sage-combinat, cwitty | Merged in: | |
Authors: | Christian Stump, Simon King | Reviewers: | David Loeffler |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #4539 #10771 #7980 | Stopgaps: |
Description (last modified by )
This patch provides the universal cyclotomic field
sage: 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; it depends on #9651 (Addition of combinatorial free module).
Apply
Change History (84)
comment:1 Changed 12 years ago by
- Cc sage-combinat added
comment:2 follow-up: ↓ 3 Changed 12 years ago by
comment:3 in reply to: ↑ 2 ; follow-up: ↓ 4 Changed 12 years ago by
Replying to 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 then
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 likeF = 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 12 years ago by
Replying to nthiery:
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 12 years ago by
Replying to craigcitro:
True. Maybe it's a bit too "cutesy," but using
n=0
might be nice -- after all, then
th cyclotomic field has roots of unity for all divisors ofn
, so this would still hold for the universal cyclotomic field andn=0
.
Mmm, any complex number is a 0-th root of unity, isn't it?
comment:6 Changed 12 years ago by
- 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 12 years ago by
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)?
- how can I embed a CombinatorialAlgebra? into another one (here: NewCyclotomicField?(m) into NewCyclotomicField?(n) for m | n)?
- how can I define a new class UniversalCyclotomicField? containing all cyclotomic fields?
- how can I define the attribute self._one in the constructor of NewCyclotomicField? properly?
comment:8 Changed 12 years ago by
- Cc cwitty added
comment:9 Changed 11 years ago by
- Status changed from needs_work to needs_review
comment:10 Changed 11 years ago by
comment:11 Changed 11 years ago by
- Description modified (diff)
comment:12 Changed 11 years ago by
- 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 11 years ago by
- 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 11 years ago by
Replying to davidloeffler:
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 11 years ago by
- 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 11 years ago by
Can some delete all but the youngest attached patch, the buildbot gets confused.
Thanks, Christian
comment:17 Changed 11 years ago by
- Description modified (diff)
comment:18 Changed 11 years ago by
Apply trac_8327_universal_cyclotomic_field-cs.patch
comment:19 Changed 11 years ago by
Apply only trac_8327_universal_cyclotomic_field-cs.patch
comment:20 Changed 11 years ago by
- 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 11 years ago by
Apply only trac_8327_universal_cyclotomic_field-cs.patch
comment:22 Changed 11 years ago by
Apply trac_8327_universal_cyclotomic_field-cs.patch
comment:23 Changed 11 years ago by
- Dependencies set to #10963
- Owner changed from davidloeffler to (none)
comment:24 Changed 11 years ago by
- Milestone changed from sage-feature to sage-4.7.2
comment:25 Changed 10 years ago by
- Dependencies #10963 deleted
comment:26 follow-up: ↓ 27 Changed 10 years ago by
- 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 10 years ago by
- Status changed from needs_work to needs_review
Replying to nthiery:
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 10 years ago by
- Description modified (diff)
Sorry, I forgot to change the ticket description.
comment:29 Changed 10 years ago by
- 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 10 years ago by
- 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 10 years ago by
- 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.
Replying to SimonKing:
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 10 years ago by
- 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 10 years ago by
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 10 years ago by
Replying to 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 10 years ago by
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 10 years ago by
- 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 10 years ago by
Replying to stumpc5:
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 10 years ago by
- 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.
Replying to SimonKing:
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 10 years ago by
(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 10 years ago by
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 10 years ago by
- 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 10 years ago by
Replying to SimonKing:
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 someElementMethods
(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 10 years ago by
Replying to stumpc5:
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 10 years ago by
Replying to SimonKing:
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 someElementMethods
(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 10 years ago by
Replying to nthiery:
Replying to SimonKing:
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 someElementMethods
(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 10 years ago by
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 10 years ago by
- 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 10 years ago by
Replying to SimonKing:
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 10 years ago by
Replying to stumpc5:
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 10 years ago by
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 10 years ago by
You import and cimport numpy. Nicolas suggest to try and lazy_import numpy instead of import (cimport should be fine).
comment:52 Changed 10 years ago by
... or lazy-import the stuff from sage/rings/universal_cyclotomic_field/all.py? I'm preparing a patch that does this.
comment:53 Changed 10 years ago by
- 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 10 years ago by
- 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 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]
Any idea why that occurs?
comment:55 in reply to: ↑ 54 ; follow-up: ↓ 56 Changed 10 years ago by
Replying to SimonKing:
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 10 years ago by
Replying to stumpc5:
UCF.is_subring(UCF) just returns
sage: UCF is UCFand as
UniversalCyclotomicField
inherits fromUniqueRepresentation
, this should returnTrue
.
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 10 years ago by
Replying to nthiery:
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 10 years ago by
- 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 10 years ago by
Replying to stumpc5:
sage: UCF.is_subring(UCF) FalseThis 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 10 years ago by
comment:61 in reply to: ↑ 60 ; follow-up: ↓ 62 Changed 10 years ago by
Replying to stumpc5:
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 10 years ago by
Replying to SimonKing:
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 10 years ago by
Replying to stumpc5:
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 10 years ago by
Ping. I think we should try to get this in shortly.
comment:65 in reply to: ↑ 64 Changed 10 years ago by
Replying to mhansen:
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 10 years ago by
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 10 years ago by
- Work issues Rebase wrt #10771. Fix one doctest. deleted
Replying to 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 ?
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 10 years ago by
Replying to stumpc5:
(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 10 years ago by
Replying to stumpc5:
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 10 years ago by
Replying to SimonKing:
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 10 years ago by
- 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 10 years ago by
- 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 10 years ago by
- Status changed from needs_work to needs_review
I've re-rebased the patch.
comment:74 Changed 10 years ago by
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 10 years ago by
- Description modified (diff)
comment:76 Changed 10 years ago by
- Dependencies changed from #4539 #10771 to #4539 #10771 #7980
comment:77 Changed 10 years ago by
Apply trac_8327-rebased_for_v5.patch, trac_8327-docfix.patch
(patchbot's confused again)
comment:78 in reply to: ↑ 75 Changed 10 years ago by
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 10 years ago by
- 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 10 years ago by
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 10 years ago by
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 10 years ago by
Replying to 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.
Alright, I will then first build and do attach another version tomorrow then.
comment:83 in reply to: ↑ 82 Changed 10 years ago by
Replying to stumpc5:
Replying to 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.
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 10 years ago by
- Description modified (diff)
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 then
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 likeF = CyclotomicField(n=Infinity)
?