Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#26239 closed defect (fixed)

QQ not isomorphic to itself

Reported by: cremona Owned by: cremona
Priority: major Milestone: sage-8.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:

Status badges

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 cremona

  • Authors set to John Cremona
  • Cc jdemeyer added
  • Component changed from interfaces to number fields

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.

comment:2 Changed 2 years ago by cremona

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

  • Status changed from needs_review to needs_work

doc is not formatted correctly (missing ::)

comment:4 Changed 2 years ago by git

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

Replying to chapoton:

doc is not formatted correctly (missing ::)

Sorry --fixed.

comment:6 Changed 2 years ago by jdemeyer

And this is kind of strange also (double "see"):

See (see :trac:`26239`)::

comment:7 Changed 2 years ago by jdemeyer

The code looks good though, although it's abusing length() a bit :-)

comment:8 Changed 2 years ago by git

  • Commit changed from ae8ebf34e3ed85c8e373def46a8a9fcd876d16a6 to 7b9db55b03537fe9c1c5c5b421ac2b1f73004831

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

7b9db55#26239: fix dosctring again

comment:9 follow-up: Changed 2 years ago by cremona

  • 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 ; follow-up: Changed 2 years ago by 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.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 2 years ago by cremona

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 jdemeyer

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 jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:14 Changed 2 years ago by cremona

Thanks!

comment:15 follow-up: Changed 2 years ago by chapoton

  • Milestone changed from sage-8.4 to sage-8.6

comment:16 in reply to: ↑ 15 Changed 2 years ago by cremona

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 chapoton

  • Milestone changed from sage-8.6 to sage-8.5

oh, well. I am pretty sure it will not make it.

comment:18 Changed 2 years ago by vbraun

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

  • Milestone changed from sage-8.5 to sage-8.6

This tickets were closed as fixed after the Sage 8.5 release.

comment:20 Changed 2 years ago by cremona

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

Last edited 2 years ago by cremona (previous) (diff)
Note: See TracTickets for help on using tickets.