Opened 3 years ago
Closed 3 years ago
#27755 closed enhancement (fixed)
some fixes and pep cleanup in number fields orders
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage8.8 
Component:  number fields  Keywords:  
Cc:  roed, tscrim, cremona, jdemeyer  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  e053068 (Commits, GitHub, GitLab)  Commit:  e053068b002e5b1a8eb56b624e507ce5de2db71d 
Dependencies:  Stopgaps: 
Description
most important change:
Some orders are built inside a smaller number field.
Change History (13)
comment:1 Changed 3 years ago by
 Branch set to u/chapoton/27755
 Commit set to ab206f2d5eea080700b3a0cc3810e1d7103df058
 Status changed from new to needs_review
comment:2 Changed 3 years ago by
 Commit changed from ab206f2d5eea080700b3a0cc3810e1d7103df058 to e053068b002e5b1a8eb56b624e507ce5de2db71d
Branch pushed to git repo; I updated commit sha1. New commits:
e053068  fixes for 27755

comment:4 Changed 3 years ago by
 Reviewers set to Travis Scrimshaw
Why did it change from V
to W
here?
 z = V.random_element()  alpha = from_V(z) + alpha = from_V(W.random_element())
So I can say every other change is good except for the behavior change noted in the ticket description, which is this change:
 # We move each element of W to this subfield.  c = alpha.coordinates_in_terms_of_powers() + # We move each generator of W to this subfield. + K, _ = K.subfield(alpha, 'beta') + gens = [K(x) for x in gens] + V, from_V, to_V = K.vector_space() + mod_gens = [to_V(x) for x in gens] + ambient = ZZ**V.dimension() + W = ambient.span(mod_gens)
For this, I would like someone who knows the theory to look at this change.
comment:5 Changed 3 years ago by
Well, I agree that an expert's eye is welcome, but this is just linear algebra.
This part of the code was just unused and plain wrong.
Do you know who we should put in cc ?
comment:6 Changed 3 years ago by
 Cc cremona jdemeyer added
John, Jeroen, could one of you have a quick look at this to make sure the change mentioned in comment:4 is okay (if you know about this part that is)?
@chapoton This is a little too far outside my area to say confidently enough that it was wrong and this is the fix. However, if it is unused, then maybe this discussion is moot since that code will never be called.
comment:7 followup: ↓ 8 Changed 3 years ago by
Well, now it is being used by the doctest whose output changed. Previously, the option allow_subfield=True
was never changing the field at all.
comment:8 in reply to: ↑ 7 Changed 3 years ago by
Replying to chapoton:
Well, now it is being used by the doctest whose output changed. Previously, the option
change_field=True
was never changing the field at all.
I see. I was misinterpreting what you meant by "unused".
comment:9 Changed 3 years ago by
The changed code looks good to me but I have not tested it on examples. The first change in Comment 4 is certainly necessary: alpha must be an element whose coordinate vector lies in W as that is the span of the gens provided. Otherwise it would be a random element of the big field K, and the whole point of this block of code is that the gens provided may lie in a proper subfield in which case the order returned is an order in that subfield.
One point caught my eye: the coordinate vectors of the gens need not be vectors of integers (this is *not* the same as checking that the egns are algebraic integers since there is no reason to suppose that the vector space basis of K is an integral basis). So W is defined to be the ZZ_span in ZZ^{n of vectors which lie in QQ}n but not necessarily ZZ^{n. I hope that works as expected. }
comment:10 Changed 3 years ago by
Seems to work fine:
sage: ambient=ZZ**2 sage: ambient.span([vector(QQ,[1/2,1/2])]) Free module of degree 2 and rank 1 over Integer Ring Echelon basis matrix: [1/2 1/2] sage: ambient.span([vector(QQ,[1/2,1/2]),vector(QQ,[1/2,1/2])]) Free module of degree 2 and rank 2 over Integer Ring Echelon basis matrix: [1/2 1/2] [ 0 1]
comment:11 Changed 3 years ago by
review, please ?
comment:12 Changed 3 years ago by
 Status changed from needs_review to positive_review
comment:13 Changed 3 years ago by
 Branch changed from u/chapoton/27755 to e053068b002e5b1a8eb56b624e507ce5de2db71d
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
some fixes and pep cleanup in number fields orders