Opened 2 years ago
Closed 17 months ago
#30518 closed defect (fixed)
eigenvectors over QQbar are incorrectly conjugated
Reported by:  Markus Wageringel  Owned by:  

Priority:  blocker  Milestone:  sage9.3 
Component:  number fields  Keywords:  
Cc:  Vincent Delecroix  Merged in:  
Authors:  Vincent Delecroix  Reviewers:  Marc Mezzarobba 
Report Upstream:  N/A  Work issues:  
Branch:  c227dad (Commits, GitHub, GitLab)  Commit:  c227dad03dbef66c5ab1b2ba3c3cd9e7155c8bed 
Dependencies:  #31628  Stopgaps: 
Description (last modified by )
Eigenvectors over QQbar could get incorrectly conjugated. In the following example from devel, the eigenvectors of a matrix over QuadraticField(1)
are computed in two ways, one of which incorrectly returns the conjugate of some eigenvectors.
By converting the matrix to QQbar
first:
sage: K.<i> = QuadraticField(1) sage: m = matrix(K, 4, [2,4*i,i,0, 4*i,2,1,0, 2*i,2,0,0, 4*i+4, 4*i4,1i,2]) sage: sorted([(e, matrix.column(vs)) for e, vs, _ in m.change_ring(QQbar).eigenvectors_right()]) [( [ 1 0] [1*I 0] [ 0 0] 2, [ 0 1] ), ( [ 1.000000000000000? + 0.?e16*I] [ 0.?e16  1.000000000000000?*I] [ 0.?e16  6.605551275463989?*I] 0.6055512754639893?, [1.000000000000000? + 1.000000000000000?*I] ), ( [ 1.000000000000000? + 0.?e17*I] [ 0.?e17  1.000000000000000?*I] [ 0.?e17 + 0.6055512754639893?*I] 6.605551275463989?, [1.000000000000000? + 1.000000000000000?*I] )]
Supposedly, this result is correct. In contrast, computing the eigenvectors directly, without first converting to QQbar
, leads to some eigenvectors getting conjugated:
sage: sorted([(QQbar(e), matrix.column(QQbar, vs)) for e, vs, _ in m.eigenvectors_right()]) [( [1 0] [I 0] [0 0] 2, [0 1] ), ( [ 1] [ 0.?e54 + 1.000000000000000?*I] [ 0.?e53 + 6.605551275463989?*I] 0.6055512754639893?, [1.000000000000000?  1.000000000000000?*I] ), ( [ 1] [ 0.?e53 + 1.000000000000000?*I] [ 0.?e52  0.6055512754639893?*I] 6.605551275463989?, [1.000000000000000?  1.000000000000000?*I] )]
Note that the QuadraticField
constructor automatically sets an embedding.
sage: K.coerce_embedding() Generic morphism: From: Number Field in i with defining polynomial x^2 + 1 with i = 1*I To: Complex Lazy Field Defn: i > 1*I
Change History (35)
comment:1 followup: 2 Changed 19 months ago by
comment:2 Changed 19 months ago by
comment:4 Changed 19 months ago by
I think this is better illustrated with
sage: for e, vs, _ in m.change_ring(QQbar).eigenvectors_right(): ....: for v in vs: ....: print(m*v == e*v) True True True True sage: for e, vs, _ in m.eigenvectors_right(): ....: for v in vs: ....: print(m*v == e*v) True True False False
comment:5 Changed 19 months ago by
The problem somehow comes from line 6638 in matrix2.pyx
m = hom(eigval.parent(), e.parent(), e)
What happens here is that the eigenvalue is quadratic over QQ
(not over the base field QQ[i]
) and that the above code does not construct the embedding that preserves the one of the base field. This can be reproduced as follows
sage: K.<i> = QuadraticField(1) sage: L = K.extension(x^2  6*x  4, 'a1') sage: eigval = L.gen() sage: eigval_conj = eigval.galois_conjugates(QQbar) sage: f0 = hom(eigval.parent(), QQbar, eigval_conj[0]) sage: f1 = hom(eigval.parent(), QQbar, eigval_conj[1]) sage: f0(i) # wrong embedding!! 0.?e54  1.000000000000000?*I sage: f1(i) # wrong embedding!! 0.?e54  1.000000000000000?*I
comment:6 Changed 19 months ago by
Could be fixed by
sage: H = Hom(eigval.parent(), QQbar) sage: f0 = H(eigval_conj[0], base_map=QQbar.coerce_map_from(K)) sage: f0(i) 0.?e54 + 1.000000000000000?*I
But maybe hom should consider the embedding of the base by default when it exists?
comment:7 Changed 19 months ago by
Authors:  → Vincent Delecroix 

Branch:  → u/vdelecroix/30518 
Commit:  → 852c89e335056d10cdefa6e25d9266c22b406e04 
Status:  new → needs_review 
comment:8 Changed 19 months ago by
Commit:  852c89e335056d10cdefa6e25d9266c22b406e04 → cf668005799173205566b30be5bf0559752f14c9 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
cf66800  30518: choose coerce embedding for the base when it exists

comment:9 Changed 19 months ago by
Description:  modified (diff) 

Summary:  eigenvectors over QQbar are incorrectly conjugated → homomorphism of extension fields do not preserve canonical embeddings of the base 
comment:10 Changed 19 months ago by
Status:  needs_review → needs_work 

This is actually only a "partial" fix. It solves the following (that also used to fail)
sage: K.<i> = QuadraticField(1, embedding=QQbar.gen()) sage: m = matrix(K, 4, [2,4*i,i,0, 4*i,2,1,0, 2*i,2,0,0, 4*i+4, 4*i4,1i,2]) sage: for e, vs, _ in m.eigenvectors_right(): ....: for v in vs: ....: print(m*v == e*v) True True True True
But since
sage: K.<i> = QuadraticField(1) sage: QQbar.coerce_map_from(K) is None True
the original example of the ticket is not solved.
comment:11 Changed 19 months ago by
Authors:  Vincent Delecroix 

Branch:  u/vdelecroix/30518 
Commit:  cf668005799173205566b30be5bf0559752f14c9 
Dependencies:  → #31558 
Description:  modified (diff) 
Summary:  homomorphism of extension fields do not preserve canonical embeddings of the base → eigenvectors over QQbar are incorrectly conjugated 
comment:13 followup: 14 Changed 18 months ago by
comment:14 Changed 18 months ago by
comment:15 Changed 18 months ago by
To my mind, the following should raise an error
sage: K.<i> = QuadraticField(1) sage: QQbar(i)
(as there is no specified embedding in QQbar)
comment:16 Changed 18 months ago by
Authors:  → Vincent Delecroix 

Branch:  → u/vdelecroix/30518 
Commit:  → 0b387f7f9b9189e3cffefb7c8ba8ea81eefb4b0e 
Dependencies:  #31558 
Status:  needs_work → needs_review 
New commits:
0b387f7  30518: raise errors in eigenspace/eigenvector in more cases

comment:18 Changed 18 months ago by
But QuadraticField(1)
has a coerce_embedding
into CC
. So it seems to me that the issue with the original example is that the conversion to QQbar
ignores the embedding. If it did not do that, the conversion could be declared as a coercion, even if with the coerce_embedding
going to CC
.
comment:19 Changed 18 months ago by
To my mind, the way number fields handle their coercions is far beyond the scope of this ticket. Even though I definitely agree that it is problematic. As soon as QQbar.has_coerce_map_from(K)
answers True
the eigenvalue/eigenvector code will work.
The minimal fix I provided turns wrong answers to errors. Together with 31551 and #31558, wrong answers are turned into right answers when coerce embeddings to the algebraic closure are available.
comment:20 Changed 18 months ago by
Reviewers:  → Marc Mezzarobba 

Ok, I tried to fix the problematic conversion and make it a coercion in #31628. You can set the present ticket to positive_review on my behalf if you want, or merge in my branch and adapt the tests.
comment:21 Changed 18 months ago by
Dependencies:  → #31628 

Status:  needs_review → needs_work 
comment:22 Changed 18 months ago by
Commit:  0b387f7f9b9189e3cffefb7c8ba8ea81eefb4b0e → c227dad03dbef66c5ab1b2ba3c3cd9e7155c8bed 

comment:23 Changed 18 months ago by
Status:  needs_work → needs_review 

comment:24 Changed 18 months ago by
Actually, some of my changes made sense (were more generic, only relying on the existence of algebraic closure and not assuming that it was QQbar). I opened #31631.
comment:25 Changed 18 months ago by
I am not sure I understand: do you want to drop your original fix (0b387f7) from this ticket and recycle it in #31631? That's fine with me, but we could also keep 0b387f7 and just update it to use number fields with no complex embedding.
comment:26 Changed 18 months ago by
I want to drop it here (since it is not necessary anymore). However, going through .algebraic_closure
(instead of hard coding QQbar
) and checking coercion to the algebraic closure make sense. The reason why I opened #31631. I also would like to add examples related to finite field, reason why I opened an independent ticket.
comment:27 Changed 18 months ago by
Status:  needs_review → positive_review 

comment:29 Changed 18 months ago by
Priority:  major → blocker 

comment:30 Changed 18 months ago by
Does the fix here provide inspiration for these somewhat similar issues?
 sagesupport, 202103, NumberFieldEmbedding gets confused with relative number fields
 #22008: complex_embedding on relative number fields is inconsistent with the base field
 #17524: polynomial for relative number field elements
comment:31 Changed 18 months ago by
Branch:  u/vdelecroix/30518 → c227dad03dbef66c5ab1b2ba3c3cd9e7155c8bed 

Resolution:  → fixed 
Status:  positive_review → closed 
comment:32 Changed 18 months ago by
Commit:  c227dad03dbef66c5ab1b2ba3c3cd9e7155c8bed 

Resolution:  fixed 
Status:  closed → new 
comment:33 Changed 18 months ago by
Branch:  c227dad03dbef66c5ab1b2ba3c3cd9e7155c8bed → u/vdelecroix/30518 

Commit:  → c227dad03dbef66c5ab1b2ba3c3cd9e7155c8bed 
Status:  new → needs_review 
comment:34 Changed 18 months ago by
Status:  needs_review → positive_review 

Move to positive review since its dependency has been fixed.
comment:35 Changed 17 months ago by
Branch:  u/vdelecroix/30518 → c227dad03dbef66c5ab1b2ba3c3cd9e7155c8bed 

Resolution:  → fixed 
Status:  positive_review → closed 
Possibly related: