#26239 closed defect (fixed)
QQ not isomorphic to itself
Reported by:  cremona  Owned by:  cremona 

Priority:  major  Milestone:  sage8.6 
Component:  number fields  Keywords:  number field isomorphism 
Cc:  jdemeyer  Merged in:  
Authors:  John Cremona  Reviewers:  Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  7b9db55 (Commits, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description
sage: Qx = PolynomialRing(QQ,'x') sage: F1 = NumberField(Qx([0,1]),'a1'); F1 Number Field in a1 with defining polynomial x sage: F2 = NumberField(Qx([0,1]),'a2'); F2 Number Field in a2 with defining polynomial x sage: F1.is_isomorphic(F2) False
The bug is because of this:
sage: f1=F1.pari_polynomial(); f1 x sage: f2=F2.pari_polynomial(); f2 x sage: f1.nfisisom(f2) [0] sage: f1.nfisisom(f2) ==0 True
As far as I can see this problem will only arise when both fields are QQ, with defining polynomial 'x'. So pari correctly finds an isomorphism but Sage thinks that paris's [0] is the same as pari's 0, the latter being what is returned when the fields are not isomorphic.
Change History (20)
comment:1 Changed 2 years ago by
 Cc jdemeyer added
 Component changed from interfaces to number fields
comment:2 Changed 2 years ago by
 Branch set to u/cremona/26239
 Commit set to 3f14c0856bb02b5e300f12cce2cc38f8d6c35abf
 Keywords number added; pari interface removed
 Status changed from new to needs_review
New commits:
3f14c08  #26239: fix is_isomorphism in trivial case of QQ

comment:3 followup: ↓ 5 Changed 2 years ago by
 Status changed from needs_review to needs_work
doc is not formatted correctly (missing ::
)
comment:4 Changed 2 years ago by
 Commit changed from 3f14c0856bb02b5e300f12cce2cc38f8d6c35abf to ae8ebf34e3ed85c8e373def46a8a9fcd876d16a6
Branch pushed to git repo; I updated commit sha1. New commits:
ae8ebf3  #26239: fix dosctring

comment:5 in reply to: ↑ 3 Changed 2 years ago by
comment:6 Changed 2 years ago by
And this is kind of strange also (double "see"):
See (see :trac:`26239`)::
comment:7 Changed 2 years ago by
The code looks good though, although it's abusing length()
a bit :)
comment:8 Changed 2 years ago by
 Commit changed from ae8ebf34e3ed85c8e373def46a8a9fcd876d16a6 to 7b9db55b03537fe9c1c5c5b421ac2b1f73004831
Branch pushed to git repo; I updated commit sha1. New commits:
7b9db55  #26239: fix dosctring again

comment:9 followup: ↓ 10 Changed 2 years ago by
 Status changed from needs_work to needs_review
sorry again, fixed again. JD: I am using t.length() to find the length of the pari list t, so why do you call that an abuse?
comment:10 in reply to: ↑ 9 ; followup: ↓ 11 Changed 2 years ago by
Replying to cremona:
I am using t.length() to find the length of the pari list t, so why do you call that an abuse?
You are also calling it on the integer 0
which happens to have length 0.
comment:11 in reply to: ↑ 10 ; followup: ↓ 12 Changed 2 years ago by
Replying to jdemeyer:
Replying to cremona:
I am using t.length() to find the length of the pari list t, so why do you call that an abuse?
You are also calling it on the integer
0
which happens to have length 0.
OK, I see. I forgot that when fields are not isomorphic pari returns 0 and not the empty list. Feel free to suggest an alternative fix which tests for 0 in a way which does not also match [0].
comment:12 in reply to: ↑ 11 Changed 2 years ago by
Replying to cremona:
I forgot that when fields are not isomorphic pari returns 0 and not the empty list.
That's the underlying reason for the bug that we're trying to fix.
Feel free to suggest an alternative fix which tests for 0 in a way which does not also match [0].
Checking the length is partially a hack and partially elegant at the same time. I would have checked the type (t.type() == "t_VEC"
) but I'm fine your proposal.
comment:13 Changed 2 years ago by
 Reviewers set to Jeroen Demeyer
 Status changed from needs_review to positive_review
comment:14 Changed 2 years ago by
Thanks!
comment:15 followup: ↓ 16 Changed 2 years ago by
 Milestone changed from sage8.4 to sage8.6
comment:16 in reply to: ↑ 15 Changed 2 years ago by
Replying to chapoton:
Why not 8.5 as it's a tiny change which fixes a mathematically incorrect output?
comment:17 Changed 2 years ago by
 Milestone changed from sage8.6 to sage8.5
oh, well. I am pretty sure it will not make it.
comment:18 Changed 2 years ago by
 Branch changed from u/cremona/26239 to 7b9db55b03537fe9c1c5c5b421ac2b1f73004831
 Resolution set to fixed
 Status changed from positive_review to closed
comment:19 Changed 2 years ago by
 Milestone changed from sage8.5 to sage8.6
This tickets were closed as fixed after the Sage 8.5 release.
comment:20 Changed 2 years ago by
 Commit 7b9db55b03537fe9c1c5c5b421ac2b1f73004831 deleted
 Owner changed from (none) to cremona
(comment added in error, I see taht this has been merged, just not in 8.5)
Working on it. I am also changing the return type of the maps (which are not returned by default) since currently what is returned is a pari list of pari polynomials. As far as I can tell the is_isomorphism() method with isomorphism_maps=True is not used anywhere in the sage library.