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: sage-8.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:

Status badges

Description

most important change:

Some orders are built inside a smaller number field.

Change History (13)

comment:1 Changed 3 years ago by chapoton

  • Branch set to u/chapoton/27755
  • Commit set to ab206f2d5eea080700b3a0cc3810e1d7103df058
  • Status changed from new to needs_review

New commits:

ab206f2some fixes and pep cleanup in number fields orders

comment:2 Changed 3 years ago by git

  • Commit changed from ab206f2d5eea080700b3a0cc3810e1d7103df058 to e053068b002e5b1a8eb56b624e507ce5de2db71d

Branch pushed to git repo; I updated commit sha1. New commits:

e053068fixes for 27755

comment:3 Changed 3 years ago by chapoton

  • Cc tscrim added

green bot, please review

comment:4 Changed 3 years ago by tscrim

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

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 tscrim

  • 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 follow-up: Changed 3 years ago by chapoton

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.

Last edited 3 years ago by chapoton (previous) (diff)

comment:8 in reply to: ↑ 7 Changed 3 years ago by tscrim

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 cremona

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 ZZn of vectors which lie in QQn but not necessarily ZZn. I hope that works as expected.

comment:10 Changed 3 years ago by chapoton

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 chapoton

review, please ?

comment:12 Changed 3 years ago by cremona

  • Status changed from needs_review to positive_review

comment:13 Changed 3 years ago by vbraun

  • Branch changed from u/chapoton/27755 to e053068b002e5b1a8eb56b624e507ce5de2db71d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.