fix number fields being unique parents -- this got broken over the years

[williamstein]

For example, these are all wrong:

sage: K.<a> = NumberField(x^2 - x - 1)
sage: loads(dumps(K)) is K
False
sage: K.<a> = NumberField(x^3 - x - 1)
sage: loads(dumps(K)) is K
False
sage: K.<a> = CyclotomicField(7)
sage: loads(dumps(K)) is K
False

NOTE: To keep this project manageable for now, this ticket does not address similar issues for relative number fields, if there are any.

This is related to #10448.

Component: number fields

Author: Julian Rueth

Branch/Commit: 8b06ff7

Reviewer: Simon King, Peter Bruin

Issue created by migration from https://trac.sagemath.org/ticket/11670

[williamstein]

comment:1

When working on this ticket, I also found a potentially serious bug, which is that the parameters assume_disc_small and maximize_at_primes of a number field were both lost upon pickling and unpickling. This ticket fixes that. We used to have this bug:

sage: K.<a> = NumberField(x^3-2, assume_disc_small=True, maximize_at_primes=[2], latex_name='\\alpha', embedding=2^(1/3))
sage: K._assume_disc_small
True
sage: K._maximize_at_primes
[2]
sage: L = loads(dumps(K))
sage: L._assume_disc_small
False
sage: L._maximize_at_primes  # None

Attached patch fixes that.

[williamstein]

Attachment: trac_11670.patch.gz

[williamstein]

comment:4

I should comment on UniqueFactory and UniqueRepresentation (from sage.structure.unique*). I made some attempt to use it, but decided against it for this patch. The main reason is that I want to fully maintain backward compatibility with old pickled objects, since there are a lot of pickled number field elements in the wild, and that is really opaque using UniqueRepresentation. (Also, I am concerned using UniqueRepresentation has some efficiency issues in terms of size and later changing how pickling works, since every time you pickle an object it wraps it in its own wrapper around other stuff, leading to another level of abstraction.) I don't claim that the current approach is the easiest to maintain in the long run -- in fact, it got broke before by bad patches. But in this new patch, I've added a ton of comments and a big warning to help prevent this in the future.

[williamstein]

comment:5

I also applied the patch to sage-4.7.2.alpha1 and ran "make ptestlong" with complete success.

[simon-king-jena]

comment:6

Does that fix the issues discussed on #10448 as well?

There, I tried to fix the missing uniqueness of maximal orders in number fields using cached_method decorators. However, the most recent post on #10448 (five months ago) also mentions the problem that number fields aren't unique, and thus the maximal orders can only be as unique as the umber fields they are contained in.

[simon-king-jena]

comment:7

It turns out that this ticket is also related with #10667. Since #10667 (among other things) extends caching of homsets, the non-uniqueness of number fields then yields actual coercion errors.

[simon-king-jena]

comment:8

Some brief remarks:

• In the test for NumberField_absolute_v2, you use NumberField_absolute_v1 (should be v2, not v1). The same for NumberField_cyclotomic_v2 and NumberField_quadratic_v2.

• The format, if I am not mistaken, should be

       OUTPUT:

       - tuple of parameters that define this field; used in 
         caching and pickling 

not

       OUTPUT: 
           - tuple of parameters that define this field; used in 
             caching and pickling 

[simon-king-jena]

comment:9

One problem bugging me at #10667 remains: If one calls the method point_exact from sage.schemes.elliptic_curves.heegner with the option optimize=True then a non-unique absolute number field (namely Number Field in a with defining polynomial x^12 + 4*x^11 + 56*x^10 + 170*x^9 + 1130*x^8 + 2564*x^7 + 10791*x^6 + 18054*x^5 + 51340*x^4 + 57530*x^3 + 102986*x^2 + 53724*x + 35001) emerges.

If I can fix it then I'll post a patch here, not at #10667.

[simon-king-jena]

comment:10

Yep. Here is an explicit example exposing the missing uniqueness:

sage: N = NumberField(x^12 - 4*x^11 + 6*x^10 - 5*x^9 + 5*x^8 - 9*x^7 + 21*x^6 - 9*x^5 + 5*x^4 - 5*x^3 + 6*x^2 - 4*x + 1, 'a')
sage: M = N.optimized_representation()[0]
sage: M
Number Field in a6 with defining polynomial x^12 - 4*x^11 + 6*x^10 - 5*x^9 + 5*x^8 - 9*x^7 + 21*x^6 - 9*x^5 + 5*x^4 - 5*x^3 + 6*x^2 - 4*x + 1
sage: N is M.change_names(names='a')
False
sage: N == M.change_names(names='a')
True

I guess it can be fixed in the change_names method.

[simon-king-jena]

comment:11

Aha, it boils down to one line in N.absolute_field:

        try:
            return self.__absolute_field[names]
        except KeyError:
            pass
        except AttributeError:
            self.__absolute_field = {}
        K = NumberField(self.defining_polynomial(), names, cache=False)
        K._set_structure(maps.NameChangeMap(K, self), maps.NameChangeMap(self, K))
        self.__absolute_field[names] = K
        return K

Why is the option cache=False used in that method?

[simon-king-jena]

comment:12

I read the comment

        # Note -- never call this on a cached number field, since
        # that could eventually lead to problems.

in N._set_structure. That might be a show stopper. But what actually happens if one applies it to a cached number field?

[simon-king-jena]

comment:13

I see a potential work-around. In my example, we have

sage: N = NumberField(x^12 - 4*x^11 + 6*x^10 - 5*x^9 + 5*x^8 - 9*x^7 + 21*x^6 - 9*x^5 + 5*x^4 - 5*x^3 + 6*x^2 - 4*x + 1, 'a')
sage: M = N.optimized_representation()[0]
sage: K = M.change_names(names='a')
sage: N == K
True
sage: N is K
False
sage: N.structure()
(Ring Coercion endomorphism of Number Field in a with defining polynomial x^12 - 4*x^11 + 6*x^10 - 5*x^9 + 5*x^8 - 9*x^7 + 21*x^6 - 9*x^5 + 5*x^4 - 5*x^3 + 6*x^2 - 4*x + 1, Ring Coercion endomorphism of Number Field in a with defining polynomial x^12 - 4*x^11 + 6*x^10 - 5*x^9 + 5*x^8 - 9*x^7 + 21*x^6 - 9*x^5 + 5*x^4 - 5*x^3 + 6*x^2 - 4*x + 1)
sage: K.structure()
(Isomorphism given by variable name change map:
  From: Number Field in a with defining polynomial x^12 - 4*x^11 + 6*x^10 - 5*x^9 + 5*x^8 - 9*x^7 + 21*x^6 - 9*x^5 + 5*x^4 - 5*x^3 + 6*x^2 - 4*x + 1
  To:   Number Field in a6 with defining polynomial x^12 - 4*x^11 + 6*x^10 - 5*x^9 + 5*x^8 - 9*x^7 + 21*x^6 - 9*x^5 + 5*x^4 - 5*x^3 + 6*x^2 - 4*x + 1, Isomorphism given by variable name change map:
  From: Number Field in a6 with defining polynomial x^12 - 4*x^11 + 6*x^10 - 5*x^9 + 5*x^8 - 9*x^7 + 21*x^6 - 9*x^5 + 5*x^4 - 5*x^3 + 6*x^2 - 4*x + 1
  To:   Number Field in a with defining polynomial x^12 - 4*x^11 + 6*x^10 - 5*x^9 + 5*x^8 - 9*x^7 + 21*x^6 - 9*x^5 + 5*x^4 - 5*x^3 + 6*x^2 - 4*x + 1)

Would it make sense to declare two number fields different if they have different structure()?

[williamstein]

Author: William Stein

[williamstein]

Reviewer: Simon King

[pjbruin]

comment:33

With your changes, Sage doesn't complain anymore if the user does not specify the name of the generator, but silently names it "a":

sage: K = NumberField(x^2 + 1); K
Number Field in a with defining polynomial x^2 + 1

I don't like this; since the name is part of the defining data, I think we should insist on the user specifying the name. If you think this should be changed, I think it is better to create a new ticket so this interface issue can be discussed separately from the technical changes made in this ticket.

[sagetrac-git]

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

f1cbe03	Merge branch 'develop' into ticket/11670
8e96824	Raise an error if the generator of a number field has not been specified.

[sagetrac-git]

Changed commit from 70702aa to 8e96824

[saraedum]

comment:35

Replying to @pjbruin:

With your changes, Sage doesn't complain anymore if the user does not specify the name of the generator, but silently names it "a":

You are right. I just implemented what the docstring used to say. I fixed the implementation and the docstring.

[pjbruin]

Changed branch from u/saraedum/ticket/11670 to u/pbruin/11670-NumberField_unique

[pjbruin]

Changed reviewer from Simon King to Simon King, Peter Bruin

[pjbruin]

Changed commit from 8e96824 to 9b4deff

[pjbruin]

comment:36

Thanks. I made a reviewer patch to fix some typos, formatting and Python 3 compatibility things; I changed the error message for name=None back to what it was to fix one failing doctest.

I would be happy give this a positive review, but I think it is better if Simon or someone else with expertise in this area takes a quick look at it as well.

[mezzarobba]

Work Issues: merge conflicts with 6.2β8

[sagetrac-git]

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

8b06ff7

Merge branch 'develop' into ticket/11670-NumberField_unique

[sagetrac-git]

Changed commit from 9b4deff to 8b06ff7

[pjbruin]

New commits:

8b06ff7

Merge branch 'develop' into ticket/11670-NumberField_unique

[pjbruin]

Changed work issues from merge conflicts with 6.2β8 to none

[pjbruin]

comment:40

OK, let's not leave this to bitrot. As an additional test, I checked that #11669 (which depends on this ticket) still works correctly, and it does.

[vbraun]

Changed branch from u/pbruin/11670-NumberField_unique to 8b06ff7