Opened 8 years ago

Closed 15 months ago

# Let the `TestSuite` test that the construction of a parent returns the parent

Reported by: Owned by: SimonKing major sage-9.2 coercion construction functor, test suite, sd53 novoselt, nthiery, combinat, tscrim, chapoton, gh-mwageringel, klee, dkrenn Simon King, Matthias Koeppe, Marc Mezzarobba, Travis Scrimshaw Travis Scrimshaw, Matthias Koeppe N/A 29e2ce0 29e2ce05c43552a7e383441624fa0cfdb840e786

Andrey told me about the following problem. When he implemented toric lattices, he inherited a `.construction()` method from general lattices. Consequence: If he tried to add elements of two different toric lattices, then Sage applied a pushout construction and added the two elements after pushing them to `ZZ^2`, which was not what he wanted.

His solution was, I think, the correct one: He overloaded the `.construction()` method, so that it now returns `None`.

Update: A similar problem also showed up in #30360.

Suggestion: Introduce a test of the `TestSuite` of a parent P, that will complain if P.construction() returns a pair `F, O` such that `F(O)!=P`.

I think this test should be put into `sage.structure.parent.Parent` (Update 9.2: `sage.categories.sets_cat`), because this is where `.construction()` is defined in the first place.

### comment:1 Changed 8 years ago by SimonKing

I did not run the full test suite yet, but I already detected several bugs in `.construction()`. So, introducing this test really is a good idea.

Until now, I found:

1. Boolean polynomial rings:
```sage: P.<x0, x1, x2, x3> = BooleanPolynomialRing(4,order='degrevlex(2),degrevlex(2)')
sage: P.construction()
(MPoly[x0,x1,x2,x3], Finite Field of size 2)
sage: F, O = P.construction()
sage: F(O)
Multivariate Polynomial Ring in x0, x1, x2, x3 over Finite Field of size 2
sage: P
Boolean PolynomialRing in x0, x1, x2, x3
```
Suggested solution: A boolean polynomial ring is, after all, a quotient of a polynomial ring. So, it should be constructed as such, but the `QuotientFunctor` should be provided with an additional attribute making it notice that a boolean polynomial ring shall be returned.
1. Integer mod rings that are initialised in a non-default category:
```sage: P = IntegerModRing(19, category = Fields())
sage: F,O = P.construction()
sage: F(O)
Ring of integers modulo 19
sage: P
Ring of integers modulo 19
sage: F(O) == P
False
```
Why is this? Well, providing a different category changes the class of P (this is how the category framework works), and the `__cmp__` method of integer mod rings checks for the class. And the construction functor `F` is not aware of the category of `P`.

Suggested solutions: Change `__cmp__` such that `F(O)` evaluates equal to `P`, even though `F(O)` is not in the category of fields, or alternatively make the construction functor aware of the category, so that `F(O)` actually is in the category of fields.

### comment:2 Changed 8 years ago by SimonKing

And I am to blame for a third problem, that is actually fairly similar to the second problem mentioned above. It is in my thematic tutorial on categories and coercion.

• I create a parent class inheriting from `UniqueRepresentation`.
• The parent class has a construction functor that keeps track of the "important" arguments needed to reconstruct the parent.
• I create one instance P of the parent class using an additional "unimportant" argument, namely a category.

Consequence: When trying to reconstruct P using the construction functor, the "unimportant" argument is missing, and hence `UniqueRepresentation` believes that a new instance needs to be created. After all, for `UniqueRepresentation`, all arguments are important parts of the cache key.

I have to think how to solve this.

### comment:3 Changed 8 years ago by SimonKing

• Branch set to u/SimonKing/ticket/15223
• Created changed from 09/24/13 13:47:16 to 09/24/13 13:47:16
• Modified changed from 09/24/13 18:32:03 to 09/24/13 18:32:03

### comment:4 Changed 8 years ago by SimonKing

Is it really needed that `BooleanPolynomialRing` has a `.construction()`? Granted, one could describe it as the quotient of a polynomial ring over GF(2). However, you could actually not take an arbitrary ring R and create a boolean polynomial ring over R---the base ring will always be GF(2).

Anyway, using the construction functor for multivariate polynomial rings is plain wrong. We have two options:

1. Let the construction be None
2. Create a new construction functor for boolean polynomial rings, either similar to the multivariate polynomial ring functor, or similar to the quotient functor.

If we go for None, then we would have problems to do fancy constructions starting with a boolean polynomial ring: There would be no pushout constructions. I wonder if we need pushout constructions.

If we go for a new construction functor, I'd suggest to modify quotient functors. After all, it is a quotient in a particular implementation.

And looking at the second problem, it seems to be a general problem with quotient functors: Sometimes we want to add more information on a quotient, such as "the quotient shall belong to the category of fields" or, "the quotient shall be implemented as a boolean polynomial ring". So, we should invent a general scheme to load the quotient functor with this additional information.

### comment:5 Changed 8 years ago by SimonKing

By the way, the branch that I uploaded fixes all doctests, except for the three issues described in the previous posts (boolean polynomial rings, quotients that are pushed into a sub-category of the default, and the stuff that I wrote in the thematic tutorial).

### comment:6 Changed 8 years ago by SimonKing

The quotient functors (`sage.categories.pushout.QuotientFunctor`) eventually call the `.quo()` method when they are called. They actually do make a special case for fields that are constructed as a quotient.

I think it would be possible to store further arguments in an attribute, say, `_further_arguments`, which should be a dictionary, and then do

```   Q = R.quo(I, names=self.names, **self._further_arguments)
```

Then, one should also make the `.quo()` method accept these additional arguments.

### comment:7 Changed 8 years ago by git

• Commit set to a81fcc1072df88cc369d7aa0b7ae423fd97d7f02

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

 [changeset:a81fcc1] Make _test_construction pass in the thematic tutorial

### comment:8 Changed 8 years ago by SimonKing

• Modified changed from 09/24/13 22:10:14 to 09/24/13 22:10:14

In the current commit, I fix the example in the thematic tutorial by making the construction functor aware of the additional arguments that were originally used to construct the parent.

It works, and I think a similar idea would work for quotient functors.

### comment:10 follow-ups: ↓ 11 ↓ 12 Changed 8 years ago by SimonKing

I just added Nicolas to this ticket, since it relates not only with coercion (this is the component of this ticket) but also with categories.

In a nutshell: If P is a parent, then `P.construction()` should return None, or a pair F,O, where F is a construction functor and O is some object.

The contract is that `F(O)==P`. But nobody has checked this contract so far. The original aim of this ticket is to introduce a test.

While we are at it, I thought one could improve `QuotientFunctor`. First of all, it must allow to pass additional arguments to the quotient being constructed. For example, the construction functor for `IntegerModRing(19, category=Fields())` must know that the category is specialised. Similarly, the construction functor for `BooleanPolynomialRing(...)` must know that the result is not just a quotient of a polynomial ring, but has a special implementation.

And something else that I want to improve with the `QuotientFunctor`: Currently, it is a functor from Rings() to Rings(). But why?? Perhaps, at some point, we want to apply the `QuotientFunctor` in the context of groups!

Hence, I think it would make sense to be more precise when choosing the domain and codomain of the functor.

Today, I experimented with this idea: If Q is a quotient, and `F,O=Q.construction()`, then the quotient functor F should go from `O.category()` to `Q.category()`.

But when running the tests, it turns out that this is too narrow: Sometimes, we want to apply the quotient functor F to a ring over a different base ring.

This makes me think of the following: Would it make sense to introduce a method of categories `C.without_parameters()`, returning the join of all super-categories of C that are not instances of `CategoryWithParameters`?

For example, for `C = Category.join([EnumeratedSets(),Algebras(QQ)])`, `C.without_parameters()` would return `Join of Category of rings and Category of enumerated sets`.

Nicolas, do you think it would hurt to introduce such method here?

### comment:11 in reply to: ↑ 10 ; follow-up: ↓ 13 Changed 8 years ago by nbruin

While we are at it, I thought one could improve `QuotientFunctor`. First of all, it must allow to pass additional arguments to the quotient being constructed. For example, the construction functor for `IntegerModRing(19, category=Fields())` must know that the category is specialised.

Isn't that going to be a problem?

```sage: A=IntegerModRing(19, category=Fields())
sage: B=IntegerModRing(19)
sage: B in Fields()
True
```

After this isn't the category of `B` refined to fields? And doesn't that then mean that A and B are the same, and hence should be identical? But if UniqueRepresentation takes the category argument into account, they won't be identical.

### comment:12 in reply to: ↑ 10 ; follow-up: ↓ 14 Changed 8 years ago by nthiery

I just added Nicolas to this ticket, since it relates not only with coercion (this is the component of this ticket) but also with categories.

In a nutshell: If P is a parent, then `P.construction()` should return None, or a pair F,O, where F is a construction functor and O is some object.

The contract is that `F(O)==P`. But nobody has checked this contract so far. The original aim of this ticket is to introduce a test.

This sounds like a good idea indeed! I would tend to put the test in Sets (in the general trend that there is already too much stuff in Parent; and maybe construction should be moved there, so as to make it easier to overload it, e.g. in some categories).

It would make sense as well for the test to accept the case where `construction` is an undefined abstract method. But maybe we don't have a use case for now.

While we are at it, I thought one could improve `QuotientFunctor`. First of all, it must allow to pass additional arguments to the quotient being constructed. For example, the construction functor for `IntegerModRing(19, category=Fields())` must know that the category is specialised. Similarly, the construction functor for `BooleanPolynomialRing(...)` must know that the result is not just a quotient of a polynomial ring, but has a special implementation.

And something else that I want to improve with the `QuotientFunctor`: Currently, it is a functor from Rings() to Rings(). But why?? Perhaps, at some point, we want to apply the `QuotientFunctor` in the context of groups!

Hence, I think it would make sense to be more precise when choosing the domain and codomain of the functor.

Today, I experimented with this idea: If Q is a quotient, and `F,O=Q.construction()`, then the quotient functor F should go from `O.category()` to `Q.category()`.

But when running the tests, it turns out that this is too narrow: Sometimes, we want to apply the quotient functor F to a ring over a different base ring.

This does not seem directly related to this ticket. Could this easily be split off into a separate ticket?

This makes me think of the following: Would it make sense to introduce a method of categories `C.without_parameters()`, returning the join of all super-categories of C that are not instances of `CategoryWithParameters`?

For example, for `C = Category.join([EnumeratedSets(),Algebras(QQ)])`, `C.without_parameters()` would return `Join of Category of rings and Category of enumerated sets`.

Nicolas, do you think it would hurt to introduce such method here?

This seems like a sensible method; so if you have a use for it, go ahead.

Cheers,

Nicolas

### comment:13 in reply to: ↑ 11 ; follow-up: ↓ 18 Changed 8 years ago by SimonKing

Isn't that going to be a problem?

```sage: A=IntegerModRing(19, category=Fields())
sage: B=IntegerModRing(19)
sage: B in Fields()
True
```

After this isn't the category of `B` refined to fields?

It is:

```sage: A = IntegerModRing(19)
sage: B = IntegerModRing(19,category=Fields())
sage: A==B
False
sage: issubclass(type(A), Fields().parent_class)
False
sage: A in Fields()
True
sage: issubclass(type(A), Fields().parent_class)
True
```

And doesn't that then mean that A and B are the same,

No, it doesn't (to my surprise, I actually thought they should be recognised as equal now):

```sage: A==B
False
sage: type(A)==type(B)
False
sage: A.category() == B.category()
False
sage: A.category()
Join of Category of fields and Category of subquotients of monoids and Category of quotients of semigroups and Category of finite enumerated sets
sage: B.category()
Join of Category of fields and Category of subquotients of monoids and Category of quotients of semigroups
```

and hence should be identical? But if UniqueRepresentation takes the category argument into account, they won't be identical.

No, it is not UniqueRepresentation:

```sage: isinstance(A,UniqueRepresentation)
False
```

### comment:14 in reply to: ↑ 12 Changed 8 years ago by SimonKing

This sounds like a good idea indeed! I would tend to put the test in Sets (in the general trend that there is already too much stuff in Parent; and maybe construction should be moved there, so as to make it easier to overload it, e.g. in some categories).

I am not so sure about this. `.construction()` is one of the places where mathematics and implementation meet: On the one hand, `.construction()` returns a functor (or at least something that pretends to be a functor), which is a mathematical notion. On the other hand, the construction functors are quite clearly also responsible for choosing implementations. For example, the fact that the following pushout uses dense power series rings is entirely due to construction functors:

```sage: Ps.<x> = PowerSeriesRing(QQ, sparse=True)
sage: Pd.<x> = PowerSeriesRing(ZZ, sparse=False)
sage: Pd['y'].has_coerce_map_from(Ps['y'])
False
sage: pushout(Ps['y'],Pd['y'])
Univariate Polynomial Ring in y over Power Series Ring in x over Rational Field
sage: pushout(Ps['y'],Pd['y']) == Ps['y']
False
```

But if it is (partially) about implementation, then I believe its place is not in `Sets.ParentMethods`.

But when running the tests, it turns out that this is too narrow: Sometimes, we want to apply the quotient functor F to a ring over a different base ring.

This does not seem directly related to this ticket. Could this easily be split off into a separate ticket?

Probably better. I am actually not sure if it would be a good idea to modify the quotient functor in that way.

Nicolas, do you think it would hurt to introduce such method here?

This seems like a sensible method; so if you have a use for it, go ahead.

Not sure yet.

### comment:15 Changed 8 years ago by SimonKing

And now I realise that I use git, and I have no idea how to pick some part of the changeset of my last commit, and move it to a different branch.

### comment:16 follow-up: ↓ 17 Changed 8 years ago by jkeitel

Hi Simon,

I am not sure how to just take a part of your last commit, but you can do the following: While you're on your current development branch, write git branch new_branch to start a new branch pointing to the current commit. Then go back to the old branch via git checkout old_branch and either delete manually the changes you don't want to to have on that branch or go back to a previous commit via git reset --hard first_few_digits_of_sha_to_revert_to You can then recommit some of the changes or just start working from there again.

This probably doesn't do exactly what you want, but it might be a start.

### comment:17 in reply to: ↑ 16 Changed 8 years ago by SimonKing

I am not sure how to just take a part of your last commit, but you can do the following: While you're on your current development branch, write git branch new_branch to start a new branch pointing to the current commit. Then go back to the old branch via git checkout old_branch and either delete manually the changes you don't want to to have on that branch or go back to a previous commit via git reset --hard first_few_digits_of_sha_to_revert_to You can then recommit some of the changes or just start working from there again.

This probably doesn't do exactly what you want, but it might be a start.

Thank you!

I had already started before your answer came. That's to say, I have stored `git diff HEAD~ HEAD` into a file `tmp.patch`, then manually reverted the changes that I didn't like, did `git add <changed_files>` and `git commit --amend`. And now, I should be able to create a branch for a different ticket, and apply the relevant changes stored in `tmp.patch`.

Since I didn't push the "wrong" commit to trac (in particular, clearly no other branch on trac will use my wrong commit), I guess it is ok to do `git commit --amend`, although it changes SHA1.

### comment:18 in reply to: ↑ 13 Changed 8 years ago by nbruin

No, it is not UniqueRepresentation:

```sage: isinstance(A,UniqueRepresentation)
False
```

That doesn't tell the entire story, though:

```sage: type(IntegerModRing).mro()
[sage.rings.finite_rings.integer_mod_ring.IntegerModFactory,
sage.structure.factory.UniqueFactory,
sage.structure.sage_object.SageObject,
object]
```

so perhaps the object isn't really UniqueRepresentation but the system tries to implement the semantics via a factory. So I think you're seeing exactly the problem I was afraid of:

```sage: A1=IntegerModRing(19)
sage: A2=IntegerModRing(19)
sage: B1=IntegerModRing(19,category=Fields())
sage: B2=IntegerModRing(19,category=Fields())
sage: A1 is A2
True
sage: B1 is B2
True
sage: A1 in Fields()
True
sage: type(A1)
<class 'sage.rings.finite_rings.integer_mod_ring.IntegerModRing_generic_with_category'>
sage: type(B1)
<class 'sage.rings.finite_rings.integer_mod_ring.IntegerModRing_generic_with_category'>
sage: type(A1) == type(B1)
False
```

so after this we do have two functionally equivalent copies of the same ring: `A1,B1`, but they are not identical. Furthermore, their types are not identical or equal either, but it's hard to see what the difference is between them (note `A1.__cmp__` to see that this last `False` is the reason why `A1 != B1`.

Allowing categories to be refined on "global" (uniqueified) objects implies that the `category` argument needs to be ignored on lookup during the uniqueification. You can't really control the category anyway:

```sage: A1=IntegerModRing(19,category=Rings())
sage: A1.category()
Join of Category of commutative rings and Category of subquotients of monoids and Category of quotients of semigroups
sage: A1 in Fields()
True
sage: A2=IntegerModRing(19,category=Rings())
sage: A2.category()
Join of Category of subquotients of monoids and Category of quotients of semigroups and Category of fields
```
Last edited 8 years ago by nbruin (previous) (diff)

### comment:19 follow-up: ↓ 20 Changed 8 years ago by nbruin

Should specifying the category even be allowed anyway? What can be achieved by doing so? You can get horrible nonsense:

```sage: A=IntegerModRing(16,category=Fields())
sage: P.<x>=A[]
sage: P in UniqueFactorizationDomains()
True
sage: (2*x+1)^8
1
```

Of course if you do this, you're just asking for the insanity. Normally everything is fine:

```sage: B=IntegerModRing(16)
sage: Q.<x>=B[]
sage: Q in UniqueFactorizationDomains()
False
```

But it does mean that if `B` here were to be identical to `A`, then the first example would permanently poison the sage session with nonsense. So I'd think `IntegerModRing(16,category=Fields())` should produce an error.

Last edited 8 years ago by nbruin (previous) (diff)

### comment:20 in reply to: ↑ 19 Changed 8 years ago by SimonKing

But it does mean that if `B` here were to be identical to `A`, then the first example would permanently poison the sage session with nonsense.

And that's why I think it is a good idea that all given arguments, including the category, are taken into the cache key of the factory. That way, it won't matter if you put a non-field into the category of fields, because you are still able to create a "sane" version of the same ring.

So I'd think `IntegerModRing(16,category=Fields())` should produce an error.

I don't think so. And in any case, it should be on a different ticket.

### comment:21 follow-up: ↓ 22 Changed 8 years ago by SimonKing

Hm. But the more I think about it...:

In the `__cmp__` method of `IntegerModRing`, it is said:

```        if type(other) is not type(self):   # so that GF(p) =/= Z/pZ
return cmp(type(self), type(other))
return cmp(self.__order, other.__order)
```

Note the comment: The aim is to let `GF(p)` and `Z/pZ` evaluate unequal. This gives rise to two questions:

1. Do we really want that they evaluate unequal?

We have

```sage: GF(5).has_coerce_map_from(IntegerModRing(5))
True
sage: IntegerModRing(5).has_coerce_map_from(GF(5))
False
```

Since there is no coercion in both directions, the two rings can not be equal. So, I'd say `GF(5)!=IntegerModRing(5)` is correct, or at least consistent.

1. Do we really want that `IntegerModRing(5)` and `IntegerModRing(5,category=Fields())` evaluate unequal?

This time, the coercions exist in both directions:

```sage: IntegerModRing(5).has_coerce_map_from(IntegerModRing(5, category=Fields()))
True
sage: IntegerModRing(5, category=Fields()).has_coerce_map_from(IntegerModRing(5))
True
```

Hence, we might want to let them evaluate equal.

I see the following options:

• Make it so that integer mod rings with the same modulus evaluate equal, regardless of their type and category. This would be in spite of the comment in the code of `__cmp__`.
• Change `__cmp__` so that integer mod rings evaluate equal if and only if there is a coercion in both directions. Hence, `GF(5)` and `ZZ.quo(5)` would evaluate unequal, but `IntegerModRing(5, category=...)` would all evaluate equal, independent of the category.
• Change `_coerce_map_from_`, so that there is a coercion in both directions if and only if the two integer mod rings evaluate equal. Hence, `IntegerModRing(5, category=...)` would all be distinct, for different categories.
• Do not allow to put in a "category" argument. But then, we might want to learn the rationale of introducing the argument in the first place. After all, if the modulus of integer mod ring R is a prime number, then asking `R in Fields()` will return true. So, if one knows during creation of the ring that the modulus is prime, one might use `GF(p)` instead of `IntegerModRing(p,category=Fields())`.

Further ways to go? In any case, there should be a new ticket.

Last edited 8 years ago by SimonKing (previous) (diff)

### comment:22 in reply to: ↑ 21 Changed 8 years ago by SimonKing

• Do not allow to put in a "category" argument. But then, we might want to learn the rationale of introducing the argument in the first place. After all, if the modulus of integer mod ring R is a prime number, then asking `R in Fields()` will return true. So, if one knows during creation of the ring that the modulus is prime, one might use `GF(p)` instead of `IntegerModRing(p,category=Fields())`.

How embarrassing!

I just used `git blame` to find out who introduced the use of a "category" argument for integer mod rings---and found that it was I, namely in #9138.

I hope #9138 gives a rationale...

### comment:23 Changed 8 years ago by SimonKing

`IntegerModRing` is not mentioned in the discussion of #9138. But in the patch, I found that providing the category as an additional argument used to be a "todo". Namely, before #9138, we had this:

```sage: FF = IntegerModRing(17, category = Fields()) # todo: not implemented
sage: FF.category()
Join of Category of fields and Category of finite enumerated sets
sage: TestSuite(FF).run()                          # todo: not implemented
```

So, this yields to the question: Why has it been a "todo"? Nicolas, did you added this "todo"?

### comment:24 Changed 8 years ago by SimonKing

It seems that it became "todo" in #8562. And there is a discussion here. It seems indeed that in this discussion there was an agreement that

1. one should not do primality test when creating the `IntegerModRing`
2. calling `is_field()` will do a primality test, and it was even suggested to update the category to `Fields()`, which actually has been impossible at that time
3. it should be possible for the user to assert that the modulus is prime, without using `GF(n)`. And, by the way, note that `GF(n)` would by default do a factorisation of `n`, and I don't know if it is possible for the user to skip this.

What do I conclude from this?

• Given 3., I see why people want to work with `IntegerModRing(n,category=Fields())` instead of `GF(n)` when they know that the huge number n is prime.
• Given 2., it might make sense to call `_refine_category` in `is_field` (and not only by 'R in Fields()') when the primality test succeeds.
• Given the cited discussion, it seems that the existence of an optional `category` argument is good.
• Providing the category `Fields()` at creation time should have the same effect as refining the category to `Fields()`. Namely, at the moment, the categories of `IntegerModRing(5,category=Fields())` and `IntegerModRing(5)` after `... in Fields()` are different.

And I am stuck with the following questions:

• Should the choice of a category play a role in testing equality for integer mod rings?
• Would we like that there is precisely one instance of an integer mod ring for a given modulus, that is automatically updated if the user later provides more information? What I mean is this:
```sage: R = IntegerModRing(5)
sage: R.category()
Join of Category of commutative rings and Category of subquotients of monoids and Category of quotients of semigroups and Category of finite enumerated sets
sage: S = IntegerModRing(5, category=Fields())
sage: R is S
True
sage: R.category()
Join of Category of fields and Category of subquotients of monoids and Category of quotients of semigroups and Category of finite enumerated sets
```

Suggested way to proceed

• I open a new ticket implementing the conclusions formulated above.
• I make these one or two tickets a dependency for this ticket.

### comment:25 Changed 8 years ago by SimonKing

• Dependencies set to #15229

I have opened #15229 for the new problems.

### comment:26 Changed 8 years ago by SimonKing

• Keywords construction functor test suite sd53 added

### comment:27 Changed 8 years ago by git

• Commit changed from a81fcc1072df88cc369d7aa0b7ae423fd97d7f02 to cb4929aa68113ebdfe4e471a014f4621e42cdba7

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

 [changeset:cb4929a] Rebase wrt. #15229. Fix remaining doc test errors. [changeset:11999a2] Merge branch 'ticket/15229' into ticket/15223 [changeset:2a08a44] Make pickling of ZZ/nZZ aware of being in Fields() [changeset:6ada7e0] Raise an error if a non-prime integer mod ring is found in Fields() [changeset:cdc6806] Replace optional "category" argument by "is_field" [changeset:75576e2] Warn if an integer mod ring is mistakenly found in Fields() [changeset:1d2e2bc] Document the "proof" argument of is_field. [changeset:154f982] Comparison of ZZ/nZZ disregards category Also refine category when calling is_field() [changeset:e83735c] Provide several constr. functors with more parameters

### comment:28 Changed 8 years ago by SimonKing

• Authors set to Simon King
• Status changed from new to needs_review

#15229 is ready for review (hint). I have merged #15229 into this branch, and rebased this branch, which was needed, as #15229 introduced a new convention for teaching to an integer mod ring that it is a field.

For me all tests pass. Hence, this one is ready for review, too!

Last edited 8 years ago by SimonKing (previous) (diff)

### comment:29 Changed 8 years ago by mmezzarobba

• Status changed from needs_review to needs_work

There are new test failures with the current `develop`, unfortunately:

```----------------------------------------------------------------------
sage -t src/sage/rings/polynomial/pbori.pyx  # 1 doctest failed
sage -t src/sage/rings/finite_rings/integer_mod_ring.py  # 4 doctests failed
sage -t src/sage/rings/residue_field.pyx  # 2 doctests failed
sage -t src/sage/rings/finite_rings/finite_field_pari_ffelt.py  # 1 doctest failed
----------------------------------------------------------------------
```

### comment:30 Changed 8 years ago by SimonKing

Reviewing the errors:

```sage -t src/sage/rings/polynomial/pbori.pyx
**********************************************************************
File "src/sage/rings/polynomial/pbori.pyx", line 476, in sage.rings.polynomial.pbori.BooleanPolynomialRing.construction
Failed example:
P.<x0, x1, x2, x3> = BooleanPolynomialRing(4,order='degrevlex(2),degrevlex(2)')
Expected nothing
Got:
doctest:1: DeprecationWarning: using 'degrevlex' in Boolean polynomial rings is deprecated. If needed, reverse the order of variables manually and use 'degneglex'
See http://trac.sagemath.org/13849 for details.
**********************************************************************
```

OK, it seems we need a different ordering. Mostly harmless

`sage -t src/sage/rings/residue_field.pyx` and `sage -t src/sage/rings/finite_rings/finite_field_pari_ffelt.py`: The new test fails on some examples. Hooray! The new test helped to detect another bug!

For me, `sage -t src/sage/rings/finite_rings/integer_mod_ring.py` passes without error.

### comment:31 follow-up: ↓ 32 Changed 8 years ago by SimonKing

PS: What has failed for you in `sage -t src/sage/rings/finite_rings/integer_mod_ring.py`?

### comment:32 in reply to: ↑ 31 ; follow-ups: ↓ 33 ↓ 36 Changed 8 years ago by mmezzarobba

PS: What has failed for you in `sage -t src/sage/rings/finite_rings/integer_mod_ring.py`?

The failures do not look deterministic. With the current state of your branch, all tests passed when I did `sage -t integer_mod_ring.py` for the first time. Then I tried again and repeatedly got:

```\$ sage -t src/sage/rings/finite_rings/integer_mod_ring.py
Running doctests with ID 2013-12-28-16-42-51-6b28af73.
Doctesting 1 file.
sage -t src/sage/rings/finite_rings/integer_mod_ring.py
**********************************************************************
File "src/sage/rings/finite_rings/integer_mod_ring.py", line 729, in sage.rings.finite_rings.integer_mod_ring.IntegerModRing_generic._pseudo_fraction_field
Failed example:
Integers(15).fraction_field()
Expected:
Traceback (most recent call last):
...
TypeError: self must be an integral domain.
Got:
Ring of integers modulo 15
**********************************************************************
1 of   7 in sage.rings.finite_rings.integer_mod_ring.IntegerModRing_generic._pseudo_fraction_field
[251 tests, 1 failure, 1.68 s]
----------------------------------------------------------------------
sage -t src/sage/rings/finite_rings/integer_mod_ring.py  # 1 doctest failed
----------------------------------------------------------------------
```

But I'm unable to reproduce this behaviour from the sage command line.

I will run the tests again after rebuilding sage from scratch.

### comment:33 in reply to: ↑ 32 ; follow-up: ↓ 34 Changed 8 years ago by mmezzarobba

I will run the tests again after rebuilding sage from scratch.

Done: I get the same heisenfailure in `integer_mod_ring.py` on your branch, without merging in develop. I'm currently checking whether the problem exists on `develop` (again with a clean build, so it will take a while).

### comment:34 in reply to: ↑ 33 ; follow-up: ↓ 35 Changed 8 years ago by mmezzarobba

I'm currently checking whether the problem exists on `develop`

Apparently it doesn't.

### comment:35 in reply to: ↑ 34 Changed 8 years ago by mmezzarobba

I'm currently checking whether the problem exists on `develop`

Apparently it doesn't.

And I'm also unable to reproduce it when I first build `develop` from scratch, and then merge in your branch and do an incremental build.

Perhaps a bug of 5.12.x that was fixed since them and resurfaced randomly in my tests due to problems with incremental builds, then?

### comment:36 in reply to: ↑ 32 Changed 8 years ago by SimonKing

The failures do not look deterministic. With the current state of your branch, all tests passed when I did `sage -t integer_mod_ring.py` for the first time. Then I tried again and repeatedly got:

```\$ sage -t src/sage/rings/finite_rings/integer_mod_ring.py
Running doctests with ID 2013-12-28-16-42-51-6b28af73.
Doctesting 1 file.
sage -t src/sage/rings/finite_rings/integer_mod_ring.py
**********************************************************************
File "src/sage/rings/finite_rings/integer_mod_ring.py", line 729, in sage.rings.finite_rings.integer_mod_ring.IntegerModRing_generic._pseudo_fraction_field
Failed example:
Integers(15).fraction_field()
Expected:
Traceback (most recent call last):
...
TypeError: self must be an integral domain.
Got:
Ring of integers modulo 15
```

Ahahahaha! It looks like the same example (modulus 15) was used in a different example to demonstrate that one can erroneously claim that `IntegerModRing(15)` is a field, and it could be that by random order of executing the tests this false information was cached. To be on the safe side, we could empty the cache.

I will run the tests again after rebuilding sage from scratch.

Did you pull from #15229?

For the record: I get

```sage -t src/sage/rings/residue_field.pyx  # 2 doctests failed
sage -t src/sage/rings/polynomial/pbori.pyx  # 1 doctest failed
sage -t src/sage/rings/finite_rings/finite_field_pari_ffelt.py  # 1 doctest failed
```

### comment:37 Changed 8 years ago by git

• Commit changed from cb4929aa68113ebdfe4e471a014f4621e42cdba7 to d0c42ff938754eb8ad52f7819671f99552d683ed

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

 ​d0c42ff `Trac 15223: Avoid side-effects between doctests of integer_mod_ring` ​d6fa391 `Merge branch 'ticket/15229' into ticket/15223` ​fae3f07 `Trac 15229: Complete a phrase in the doc, remove trailing whitespace` ​7b61761 `Merge branch 'master' into ticket/15229`

### comment:38 Changed 8 years ago by SimonKing

I have merged #15229, since it changes `integer_mod_ring.py`, and I added a commit to clear the cache of the integer mod ring factory after compromising the cache by lying about `is_field=True`.

Hopefully this will fix the "Heisenbug" (which probably is not more than a side-effect between doc-tests). The remaining failures shall be fixed in the next commit.

### comment:39 Changed 8 years ago by SimonKing

Concerning the error with residue fields:

```sage: K.<z> = CyclotomicField(7)
sage: P = K.factor(17)[0][0]
sage: ff = K.residue_field(P)
sage: ff.construction()
(AlgebraicExtensionFunctor, Finite Field of size 17)
sage: F,R = _
sage: F(R)
Finite Field in zbar of size 17^6
sage: ff
Residue field in zbar of Fractional ideal (17)
sage: ff.order()
24137569
sage: F(R).order()
24137569
```

So, the construction of a residue field just yields a finite field, not a residue field. Either the algebraic extension functor needs information of whether it is a residue field or not, or we need a "residue field functor". I wouldn't like to see the latter, since it is clearly no functor. OK, it would also be possible to kill the construction of residue fields.

### comment:40 Changed 8 years ago by SimonKing

• Work issues set to algebraic extension functor for residue fields

The algebraic extension functors already have a version to construct cyclotomic fields. It should be straight forward to make them deal with residue fields, too.

### comment:41 Changed 8 years ago by SimonKing

Rather than killing the construction, I guess the following is a nice behaviour:

```        sage: K.<z> = CyclotomicField(7)
sage: P = K.factor(17)[0][0]
sage: k = K.residue_field(P)
sage: F, R = k.construction()
sage: F
AlgebraicExtensionFunctor
sage: R
Cyclotomic Field of order 7 and degree 6
sage: F(R) is k
True
sage: F(ZZ)
Residue field of Integers modulo 17
sage: F(CyclotomicField(49))
Residue field in zeta49bar of Fractional ideal (17)
```

### comment:42 Changed 8 years ago by git

• Commit changed from d0c42ff938754eb8ad52f7819671f99552d683ed to ab2c12e1fbebf2fa75291f5542e468f952d4c9f6

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

 ​ab2c12e `Trac 15223: Construction of residue fields`

### comment:43 Changed 8 years ago by SimonKing

Residue fields are fixed with the latest commit.

New commits:

 ​ab2c12e `Trac 15223: Construction of residue fields`

New commits:

 ​ab2c12e `Trac 15223: Construction of residue fields`

### comment:44 Changed 8 years ago by git

• Commit changed from ab2c12e1fbebf2fa75291f5542e468f952d4c9f6 to b23f18d716915ae30283cd90da74f865067f72fe

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

 ​b23f18d `Trac 15223: Construction of field extensions with implementation`

### comment:45 Changed 8 years ago by SimonKing

With the new commit, the construction of field extensions is fixed.

Problem was: Finite fields may have different implementations (givaro, pari_ffelt, ...), but this was ignored in the construction of the construction functor. Now, the implementation is tracked in the construction functor.

One complication: When we merge the construction functors, we need to choose among the implementations. I decided (for now) to always rely on the default implementation upon merging.

Rationale

• The situation is not worse than before. Without the latest commit, applying the construction functor of a finite field would always use the default implementation. With the latest commit, applying the construction functor of a finite field F will use the implementation that was used for F. Only if a new construction functor is created by merging two functors (which may happen in the pushout construction), the default implementation will be used,
• We can't rely on a custom implementation in the merged functor, since a custom implementation may not be available for a non-prime field, that could be the result of merging.

### comment:46 Changed 8 years ago by SimonKing

• Status changed from needs_work to needs_review
• Work issues algebraic extension functor for residue fields deleted

Hooray, with the last commit, `sage -t src/sage/rings/polynomial/pbori.pyx` passes, too!

Hence, I think it could be reviewed now (but I will run the tests while sleeping).

### comment:47 Changed 8 years ago by git

• Commit changed from b23f18d716915ae30283cd90da74f865067f72fe to 2091e92ff6855bbb58eecbb250883b719a8d8eb0

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

 ​2091e92 `Trac 15223: Avoid deprecation warning in a new test` ​dfcf31f `Merge branch 'develop' into ticket/15223`

### comment:48 follow-up: ↓ 49 Changed 8 years ago by SimonKing

Sigh. I still don't like git. It makes it soooooo easy to do mistakes, whose reversal is considered "changing history".

Anyway. It was an accident that I did not revert merging the branch 'develop' before pushing it here.

Can someone tell me, *if* and *how* I should remove the merge commit?

But now all tests pass, and thus it can be reviewed.

### comment:49 in reply to: ↑ 48 ; follow-up: ↓ 51 Changed 8 years ago by mmezzarobba

Can someone tell me, *if* and *how* I should remove the merge commit?

Im my opinion (some people might disagree), yes, you should, since it is very unlikely that anyone has based anything on the last state of your branch. There is no need to be religious about history rewrites, one just needs to avoid forcing people to do rebases.

To remove the merge commit, just do `git rebase HEAD^ --onto HEAD^^` on your branch (and then `git push -f` to push the modified branch to trac).

### comment:50 Changed 8 years ago by git

• Commit changed from 2091e92ff6855bbb58eecbb250883b719a8d8eb0 to 7a24de6caa6abad9218c6186030787871c070905

Branch pushed to git repo; I updated commit sha1. This was a forced push. Recent commits:

 ​7a24de6 `Trac 15223: Avoid deprecation warning in a new test` ​b23f18d `Trac 15223: Construction of field extensions with implementation` ​ab2c12e `Trac 15223: Construction of residue fields` ​d0c42ff `Trac 15223: Avoid side-effects between doctests of integer_mod_ring` ​d6fa391 `Merge branch 'ticket/15229' into ticket/15223`

### comment:51 in reply to: ↑ 49 ; follow-up: ↓ 52 Changed 8 years ago by SimonKing

Can someone tell me, *if* and *how* I should remove the merge commit?

Im my opinion (some people might disagree), yes, you should, since it is very unlikely that anyone has based anything on the last state of your branch. There is no need to be religious about history rewrites, one just needs to avoid forcing people to do rebases.

To remove the merge commit, just do `git rebase HEAD^ --onto HEAD^^` on your branch (and then `git push -f` to push the modified branch to trac).

Thank you! Can you briefly explain to me the differences between `^HEAD`, `HEAD^` and `HEAD~`? I keep forgetting what is what.

### comment:52 in reply to: ↑ 51 Changed 8 years ago by mmezzarobba

Thank you! Can you briefly explain to me the differences between `^HEAD`, `HEAD^` and `HEAD~`? I keep forgetting what is what.

The last two specify revisions (commits): `rev^k` is the `k`-th parent of `rev`; `rev~k` is the grand`k`-first-parent of `rev`; `rev^` and `rev~` are shorthands `rev^1` == `rev~1`.

`^rev` is something different: it is used to specify lower bounds of ranges of revisions. For example, `rev1 rev2 ^rev3` is the set of all ancestors of `rev1` or `rev2` that are not ancestors of `rev3`.

See `git help revisions` for the full syntax of revision specifications.

### comment:53 Changed 8 years ago by vbraun_spam

• Milestone changed from sage-6.1 to sage-6.2

### comment:54 Changed 8 years ago by vbraun_spam

• Milestone changed from sage-6.2 to sage-6.3

### comment:55 Changed 8 years ago by rws

• Status changed from needs_review to needs_work
• Work issues set to wait until dependency closed

Set to 'needs work' because patchbot fails to apply the patch (open dependency).

### comment:56 Changed 7 years ago by vbraun_spam

• Milestone changed from sage-6.3 to sage-6.4

### comment:57 Changed 5 years ago by SimonKing

• Work issues changed from wait until dependency closed to Rebase

The dependency *is* closed, but apparently the branch doesn't merge.

### comment:58 Changed 15 months ago by mkoeppe

• Branch changed from u/SimonKing/ticket/15223 to u/mkoeppe/ticket/15223

### comment:59 Changed 15 months ago by mkoeppe

• Commit changed from 7a24de6caa6abad9218c6186030787871c070905 to b17280000a16b9e4d17183874b742513cc7c6e0a
• Milestone changed from sage-6.4 to sage-9.2

In the middle of manually rebasing this onto 9.2.beta10.

```DONE 91712d0545 Test that the construction of a parent really constructs the parent
DONE 91bbab6438 Add a test to Parent._test_construction
REDONE 2bb2cf269b Fix most doctest errors that are due to enlarging the test suite
DONE a81fcc1072 Make _test_construction pass in the thematic tutorial
DONE e83735ca25 Provide several constr. functors with more parameters
```

TODO:

```pick cb4929aa68 Rebase wrt. #15229. Fix remaining doc test errors.
pick d0c42ff938 Trac 15223: Avoid side-effects between doctests of integer_mod_ring
pick ab2c12e1fb Trac 15223: Construction of residue fields
pick b23f18d716 Trac 15223: Construction of field extensions with implementation
pick 7a24de6caa Trac 15223: Avoid deprecation warning in a new test
```

New commits:

 ​2b9f140 `Test that the construction of a parent really constructs the parent` ​5cbb08f `Add a test to Parent._test_construction` ​e7890f7 `Sets._test_construction: Use assertEqual` ​9939b21 `src/sage/categories: Fix doctest errors that are due to enlarging the test suite` ​0200856 `Make _test_construction pass in the thematic tutorial` ​e542531 `src/doc/en/thematic_tutorials/coercion_and_categories.rst: Fix doctest errors that are due to enlarging the test suite` ​b172800 `Provide several constr. functors with more parameters`

### comment:60 Changed 15 months ago by mkoeppe

• Description modified (diff)

### comment:61 Changed 15 months ago by mkoeppe

Looks like there is a circular import problem now:

```ImportError: cannot import name 'AlgebrasCategory' from 'sage.categories.algebra_functor' (/Users/mkoeppe/s/sage/sage-rebasing/local/lib/python3.7/site-packages/sage/categories/algebra_functor.py)
```

### comment:62 Changed 15 months ago by git

• Commit changed from b17280000a16b9e4d17183874b742513cc7c6e0a to dbe8254d521ee0555090397cc9afa8f7257d2327

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

 ​dbe8254 `Rebase wrt. #15229. Fix remaining doc test errors.`

### comment:63 Changed 15 months ago by git

• Commit changed from dbe8254d521ee0555090397cc9afa8f7257d2327 to 9a63834f61f6e24a6515688801be3ad2d07b2820

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

 ​9a63834 `Trac 15223: Avoid side-effects between doctests of integer_mod_ring`

### comment:64 Changed 15 months ago by mkoeppe

Help with this circular import business would be very welcome

### comment:65 Changed 15 months ago by mkoeppe

• Description modified (diff)

### comment:66 Changed 15 months ago by mkoeppe

• Work issues changed from Rebase to Fix circular import, test, cherry-pick the remaining 3 commits

### comment:67 Changed 15 months ago by tscrim

In `polynomial_quotient_ring.py`, a way around the problem is to just make those imports locally where you need them in `construction()`. Welcome to Sage's circular import hell. `:P`

Also

```
-        TESTS:
+        TESTS::

sage: M2 = PowerSeriesRing(QQ,4,'f', sparse=True)
sage: M == M2
False
```

### comment:68 Changed 15 months ago by git

• Commit changed from 9a63834f61f6e24a6515688801be3ad2d07b2820 to e4cab3f7e78c939aa813d24db6de33a11e42586c

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

 ​e4cab3f `WIP: Resolve circular imports`

### comment:69 Changed 15 months ago by mkoeppe

Hm... this alone did not do the trick

### comment:70 Changed 15 months ago by git

• Commit changed from e4cab3f7e78c939aa813d24db6de33a11e42586c to bed14eb5742a2d04b1a4c9f015896294c0fc0620

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

 ​bed14eb `Fix circular import problem`

### comment:71 Changed 15 months ago by git

• Commit changed from bed14eb5742a2d04b1a4c9f015896294c0fc0620 to 754a93666e16759bb3e59757bf8c591828f03434

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

 ​754a936 `Fix circular import problem`

OK, got it.

### comment:73 Changed 15 months ago by git

• Commit changed from 754a93666e16759bb3e59757bf8c591828f03434 to 96b60b5ea7c48cfcce26dc5cd68080bdfe16d73b

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

 ​96b60b5 `src/sage/rings/multi_power_series_ring.py: Fix doctest markup`

### comment:74 Changed 15 months ago by git

• Commit changed from 96b60b5ea7c48cfcce26dc5cd68080bdfe16d73b to 8d7982fe2fa304c13c2692c16ea02ed15474453b

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

 ​8d7982f `src/sage/rings/ring.pyx: Fix up merge`

### comment:75 Changed 15 months ago by mkoeppe

• Authors changed from Simon King to Simon King, Matthias Koeppe
• Status changed from needs_work to needs_review
• Work issues changed from Fix circular import, test, cherry-pick the remaining 3 commits to test, cherry-pick the remaining 3 commits

### comment:76 Changed 15 months ago by mkoeppe

Setting it to needs_review so that the patchbot runs

### comment:77 Changed 15 months ago by git

• Commit changed from 8d7982fe2fa304c13c2692c16ea02ed15474453b to ab798f60a2a7dcff925443fdb0f7b84bad447504

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

 ​ab798f6 `Trac 15223: Construction of residue fields`

### comment:78 Changed 15 months ago by git

• Commit changed from ab798f60a2a7dcff925443fdb0f7b84bad447504 to e509079faf1e6aa11ed08b378a314ce01ac1fa41

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

 ​e509079 `Fix remaining doctest errors that are due to enlarging the test suite`

### comment:79 Changed 15 months ago by git

• Commit changed from e509079faf1e6aa11ed08b378a314ce01ac1fa41 to a3896c5a8cb91894c7c70cd789857b6410481b67

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

 ​a3896c5 `Trac 15223: Construction of field extensions with implementation`

### comment:80 Changed 15 months ago by git

• Commit changed from a3896c5a8cb91894c7c70cd789857b6410481b67 to efb55acc9247d4fe44ffe444b30c695362709df8

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

 ​efb55ac `Trac 15223: Avoid deprecation warning in a new test`

### comment:81 Changed 15 months ago by git

• Commit changed from efb55acc9247d4fe44ffe444b30c695362709df8 to 2b9f40d9fe343311178d58ce8c51c1fced97998e

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

 ​2b9f40d `src/sage/categories/pushout.py: py3 fix (iteritems)`

### comment:82 Changed 15 months ago by mkoeppe

• Dependencies #15229 deleted
• Status changed from needs_review to needs_work
• Work issues test, cherry-pick the remaining 3 commits deleted

As expected, there are a number of `AssertionError: the object's construction does not recreate this object` errors.

In addition, there are some other errors, probably because of faulty rebasing:

```sage -t --random-seed=0 src/sage/rings/polynomial/polynomial_quotient_ring.py
sage -t --random-seed=0 src/sage/rings/multi_power_series_ring_element.py
sage -t --random-seed=0 src/sage/rings/multi_power_series_ring.py
sage -t --random-seed=0 src/sage/rings/power_series_ring_element.pyx
sage -t --random-seed=0 src/sage/rings/finite_rings/element_pari_ffelt.pyx
```

### comment:83 Changed 15 months ago by mkoeppe

```File "src/sage/rings/multi_power_series_ring.py", line 504, in sage.rings.multi_power_series_ring.MPowerSeriesRing_generic.construction
Failed example:
c(R)
Exception raised:
Traceback (most recent call last):
File "/home/sagemath/sage-9.1/local/lib/python3.7/site-packages/sage/doctest/forker.py", line 715, in _run
self.compile_and_execute(example, compiler, test.globs)
File "/home/sagemath/sage-9.1/local/lib/python3.7/site-packages/sage/doctest/forker.py", line 1139, in compile_and_execute
exec(compiled, globs)
File "<doctest sage.rings.multi_power_series_ring.MPowerSeriesRing_generic.construction[3]>", line 1, in <module>
c(R)
File "sage/categories/functor.pyx", line 383, in sage.categories.functor.Functor.__call__ (build/cythonized/sage/categories/functor.c:3146)
y = self._apply_functor(self._coerce_into_domain(x))
File "/home/sagemath/sage-9.1/local/lib/python3.7/site-packages/sage/categories/pushout.py", line 2456, in _apply_functor
return R.completion(self.p, self.prec, extras)
File "sage/rings/polynomial/multi_polynomial_ring_base.pyx", line 184, in sage.rings.polynomial.multi_polynomial_ring_base.MPolynomialRing_base.completion (build/cythonized/sage/rings/polynomial/multi_polynomial_ring_base.c:5003)
def completion(self, names, prec=20):
TypeError: completion() takes at most 2 positional arguments (3 given)
**********************************************************************
```

The method `MPolynomialRing_base.completion` lost its `extras` parameter in #23377/#26773, so this method is no longer compatible with other methods...

Last edited 15 months ago by mkoeppe (previous) (diff)

### comment:84 Changed 15 months ago by mkoeppe

```\$ git grep 'def completion('
src/sage/combinat/finite_state_machine.py:    def completion(self, sink=None):
src/sage/rings/function_field/function_field.py:    def completion(self, place, name=None, prec=None, gen_name=None):
src/sage/rings/integer_ring.pyx:    def completion(self, p, prec, extras = {}):
src/sage/rings/number_field/number_field.py:    def completion(self, p, prec, extras={}):
src/sage/rings/polynomial/laurent_polynomial_ring.py:    def completion(self, p, prec=20, extras=None):
src/sage/rings/polynomial/multi_polynomial_ring_base.pyx:    def completion(self, names, prec=20):
src/sage/rings/polynomial/polynomial_ring.py:    def completion(self, p, prec=20, extras=None):
src/sage/rings/qqbar.py:    def completion(self, p, prec, extras={}):
src/sage/rings/qqbar.py:    def completion(self, p, prec, extras={}):
src/sage/rings/rational_field.py:    def completion(self, p, prec, extras = {}):
```

### comment:85 Changed 15 months ago by git

• Commit changed from 2b9f40d9fe343311178d58ce8c51c1fced97998e to cf7a74be31176a3d956250b5523c973942db22b3

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

 ​49abf7b `CompletionFunctor: Add type to extras only if non-None` ​cf7a74b `MPolynomialRing_base.completion: Re-add parameter extras, pass it to PowerSeriesRing`

### comment:86 Changed 15 months ago by git

• Commit changed from cf7a74be31176a3d956250b5523c973942db22b3 to 75aaf3cc77fb030554ad306850cb233ffaccc8e7

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

 ​75aaf3c `src/sage/rings/asymptotic: Fix remaining doctest errors that are due to enlarging the test suite`

### comment:87 Changed 15 months ago by git

• Commit changed from 75aaf3cc77fb030554ad306850cb233ffaccc8e7 to 1f40071a7209de2d1d8891e7898219a51c235117

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

 ​1f40071 `Merge tag '9.2.beta11' into t/15223/ticket/15223`

### comment:88 Changed 15 months ago by git

• Commit changed from 1f40071a7209de2d1d8891e7898219a51c235117 to 69b95d353d2188195c24e42231a0db1bfd0281d5

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

 ​69b95d3 `src/sage/categories/sets_cat.py: Get rid of __cmp__ in a doctest`

### comment:89 Changed 15 months ago by git

• Commit changed from 69b95d353d2188195c24e42231a0db1bfd0281d5 to f09a79b57c6f5b7777a6d668ce7ddcf80f1e89a4

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

 ​f09a79b `FiniteField.construction: Also catch TypeError when accessing _factory_data`

### comment:90 Changed 15 months ago by mkoeppe

• Status changed from needs_work to needs_review

### comment:91 follow-up: ↓ 93 Changed 15 months ago by mkoeppe

Help with fixing the remaining failures would be welcome

### comment:93 in reply to: ↑ 91 ; follow-up: ↓ 102 Changed 15 months ago by mmezzarobba

• Branch changed from u/mkoeppe/ticket/15223 to public/ticket/15223
• Commit changed from f09a79b57c6f5b7777a6d668ce7ddcf80f1e89a4 to 8cb0422fca7b73b2b4e6a8694862967bf4905a30

Help with fixing the remaining failures would be welcome

Here is an attempt (not really tested)...

New commits:

 ​3e187de `#15223 don't try to construct polynomial quotient rings "as fields"` ​3630d25 `#15223 pass variable names to residue_field() in AlgebraicExtensionFunctor` ​8cb0422 `#15223 cartesians product functors that force a certain category`

### comment:94 Changed 15 months ago by mkoeppe

• Authors changed from Simon King, Matthias Koeppe to Simon King, Matthias Koeppe, Marc Mezzarobba

### comment:95 Changed 15 months ago by mkoeppe

`src/sage/algebras/free_zinbiel_algebra.py` needs some help

### comment:96 Changed 15 months ago by git

• Commit changed from 8cb0422fca7b73b2b4e6a8694862967bf4905a30 to 8bed5a1354aa654315507d6e40f28cb95e91a2fe

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

 ​8bed5a1 `src/sage/categories/classical_crystals.py: Fix remaining doctest errors that are due to enlarging the test suite`

### comment:97 follow-up: ↓ 100 Changed 15 months ago by tscrim

A number of those `verbose=True` tests probably are best made `verbose=False` so we don't have this problem.

Right now beta11 is compiling for me. Can you tell me what the problem is with `free_zinbiel_algebra.py` so I know what to start looking for?

### comment:98 Changed 15 months ago by git

• Commit changed from 8bed5a1354aa654315507d6e40f28cb95e91a2fe to 03f08c30f53eb22757a7c66e36601c1dc376e6bf

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

 ​03f08c3 `AlgebraicExtensionFunctor [residue]: Fixup: Pass self.names`

### comment:99 follow-up: ↓ 101 Changed 15 months ago by tscrim

Also relevant: #23010.

### comment:100 in reply to: ↑ 97 ; follow-up: ↓ 116 Changed 15 months ago by mkoeppe

Right now beta11 is compiling for me. Can you tell me what the problem is with `free_zinbiel_algebra.py` so I know what to start looking for?

On this ticket:

```sage -t --long --random-seed=0 src/sage/algebras/free_zinbiel_algebra.py
**********************************************************************
File "src/sage/algebras/free_zinbiel_algebra.py", line 211, in sage.algebras.free_zinbiel_algebra.FreeZinbielAlgebra.__init__
Failed example:
TestSuite(Z).run(elements=[Z.an_element(), G[1], G[1]*G[2]*G[0]])
Expected nothing
Got:
Failure in _test_construction:
Traceback (most recent call last):
File "/home/sagemath/sage-9.1/local/lib/python3.7/site-packages/sage/misc/sage_unittest.py", line 297, in run
test_method(tester=tester)
File "/home/sagemath/sage-9.1/local/lib/python3.7/site-packages/sage/categories/sets_cat.py", line 1508, in _test_construction
FO = self.construction()
File "/home/sagemath/sage-9.1/local/lib/python3.7/site-packages/sage/algebras/free_zinbiel_algebra.py", line 498, in construction
return ZinbielFunctor(self.variable_names()), self.base_ring()
File "sage/structure/category_object.pyx", line 474, in sage.structure.category_object.CategoryObject.variable_names (build/cythonized/sage/structure/category_object.c:4392)
raise ValueError("variable names have not yet been set using self._assign_names(...)")
ValueError: variable names have not yet been set using self._assign_names(...)
------------------------------------------------------------
The following tests failed: _test_construction
```

### comment:101 in reply to: ↑ 99 Changed 15 months ago by mkoeppe

Also relevant: #23010.

See my comment there -- all commits have already merged apparently

### comment:102 in reply to: ↑ 93 ; follow-up: ↓ 106 Changed 15 months ago by mkoeppe

Here is an attempt (not really tested)... New commits:

 ​3630d25 `#15223 pass variable names to residue_field() in AlgebraicExtensionFunctor`

I have fixed one bug, but more work is needed because the interface of method `residue_field` is not very consistent:

```\$ git grep 'def residue_field'
src/sage/categories/discrete_valuation.py:        def residue_field(self):
src/sage/categories/discrete_valuation.py:        def residue_field(self):
src/sage/rings/function_field/function_field.py:    def residue_field(self, place, name=None):
src/sage/rings/function_field/function_field.py:    def residue_field(self, place, name=None):
src/sage/rings/function_field/place.py:    def residue_field(self, name=None):
src/sage/rings/function_field/place.py:    def residue_field(self, name=None):
src/sage/rings/function_field/valuation_ring.py:    def residue_field(self, name=None):
src/sage/rings/ideal.py:    def residue_field(self):
src/sage/rings/integer_ring.pyx:    def residue_field(self, prime, check = True):
src/sage/rings/laurent_series_ring.py:    def residue_field(self):
src/sage/rings/number_field/number_field.py:    def residue_field(self, prime, names=None, check=True):
src/sage/rings/number_field/number_field_ideal.py:    def residue_field(self, names=None):
src/sage/rings/number_field/order.py:    def residue_field(self, prime, names=None, check=False):
src/sage/rings/polynomial/ideal.py:    def residue_field(self, names=None, check=True):
src/sage/rings/polynomial/polynomial_ring.py:    def residue_field(self, ideal, names=None):
src/sage/rings/power_series_ring.py:    def residue_field(self):
src/sage/rings/puiseux_series_ring.py:    def residue_field(self):
src/sage/rings/rational_field.py:    def residue_field(self, p, check=True):
src/sage/rings/valuation/valuation_space.py:        def residue_field(self):
```

### comment:104 Changed 15 months ago by mkoeppe

• Status changed from needs_review to needs_work

### comment:105 Changed 15 months ago by mkoeppe

• Dependencies set to #30507

### comment:106 in reply to: ↑ 102 Changed 15 months ago by mmezzarobba

I have fixed one bug,

Uh, I said I had not really tested my patches, but I thought I had run the tests from the few most relevant files, sorry.

but more work is needed because the interface of method `residue_field` is not very consistent:

It is not clear to me on what kinds of objects this functor can be called with `residue != None`. It may be good enough for this ticket to take care of the cases that currently work...

### comment:107 Changed 15 months ago by git

• Commit changed from 03f08c30f53eb22757a7c66e36601c1dc376e6bf to 93452f57564de963ab7d32d7d669db31de64ee20

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

 ​c9145f0 `#15223 update doctest` ​93452f5 `#15223 further fixes to calls to residue_field() via AlgebraicExtensionFunctor`

### comment:108 Changed 15 months ago by git

• Commit changed from 93452f57564de963ab7d32d7d669db31de64ee20 to 0ccc5d8f48ea3c42a7230aa31046c756845712c2

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

 ​0ccc5d8 `#15223 further fixes to calls to residue_field() via AlgebraicExtensionFunctor`

### comment:109 Changed 15 months ago by git

• Commit changed from 0ccc5d8f48ea3c42a7230aa31046c756845712c2 to b6b012b9c1a1c05f858446cbf21d7aa7e5d685b1

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

 ​b6b012b `#15223 CartesianProductGrowthGroup uniqueness`

### comment:110 follow-up: ↓ 111 Changed 15 months ago by mmezzarobba

@dkrenn: Commit ​e664f08 fixes an issue with the uniqueness of cartesian product asymptotic growth groups. My fix is a bit fragile, do you have a better idea?

Last edited 15 months ago by mmezzarobba (previous) (diff)

### comment:111 in reply to: ↑ 110 Changed 15 months ago by mmezzarobba

@dkrenn: Commit b6b012b fixes an issue with the uniqueness of cartesian product asymptotic growth groups. My fix is a bit fragile, do you have a better idea?

Hmm, actually, we have the same issue with `CartesianProductPoset` itself. So I think we need a factory for them, and that's where the normalization of the category should go. But `CartesianProductPoset` can depend on arbitrary functions. I suppose one would like to ensure uniqueness at least for the standard orderings, even if posets with custom orderings are all distinct. I'm not sure how to do that best; using `UniqueFactory`, the code will be a bit unwieldy.

Last edited 15 months ago by mmezzarobba (previous) (diff)

### comment:112 Changed 15 months ago by git

• Commit changed from b6b012b9c1a1c05f858446cbf21d7aa7e5d685b1 to 1641b4e61ee28018cbdea2432fd0479907a9bff6

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

 ​e664f08 `#15223 CartesianProductGrowthGroup uniqueness` ​1641b4e `#15223 fix comparison of CartesianProduct functors after 8cb0422`

### comment:113 Changed 15 months ago by git

• Commit changed from 1641b4e61ee28018cbdea2432fd0479907a9bff6 to 6ae558073bbdd7283657adde3cea812a403f3646

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

 ​6ae5580 `QuotientRing: If implementation=='pbori', delegate to BooleanPolynomialRing`

### comment:114 follow-up: ↓ 120 Changed 15 months ago by mkoeppe

To fix failures like the one in `src/sage/combinat/symmetric_group_algebra.py`, we also need to give `AlgebraFunctor` and `GroupAlgebraFunctor` a category argument.

Do we already have a forgetful functor somewhere that can take care of such things by composing?

### comment:115 Changed 15 months ago by git

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

 ​18f6b0b `GroupSemidirectProduct: Override the functorial construction from CartesianProduct, return None`

### comment:116 in reply to: ↑ 100 Changed 15 months ago by mkoeppe

Right now beta11 is compiling for me. Can you tell me what the problem is with `free_zinbiel_algebra.py` so I know what to start looking for?

Minimal example:

```sage: algebras.FreeZinbiel(QQ, ZZ).construction()
ValueError: variable names have not yet been set using self._assign_names(...)
```

### comment:117 Changed 15 months ago by mkoeppe

To fix the failures in `src/sage/matrix/matrix_gap.pyx`, we would need to store `implementation` in `MatrixFunctor`.

### comment:118 Changed 15 months ago by git

• Commit changed from 18f6b0b282d178282f2b9bceb2e16c6b5fdb50ad to e42842090a07dab536747285dca1c0bf67c65dd4

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

 ​29f8840 `Merge branch 'public/ticket/15223' of git://trac.sagemath.org/sage into public/ticket/15223` ​e428420 `Fixing construction functor for free Zinbiel algebras with infinite vars.`

### comment:119 Changed 15 months ago by tscrim

Thank you. Sorry for taking a week to get to this.

So the problem is the extension of the free Zinbiel algebra to the infinite number of variables but the `construction()` was only implemented for the finite case. I have fixed this and some other minor things with the construction functor for handling infinite number of variables.

### comment:120 in reply to: ↑ 114 Changed 15 months ago by tscrim

To fix failures like the one in `src/sage/combinat/symmetric_group_algebra.py`, we also need to give `AlgebraFunctor` and `GroupAlgebraFunctor` a category argument.

Do we already have a forgetful functor somewhere that can take care of such things by composing?

Sort of, there is the `ForgetfulFunction_generic` in `categories/functor.pyx`. You probably will need to implement a subclass of that for this purpose.

### comment:121 Changed 15 months ago by git

• Commit changed from e42842090a07dab536747285dca1c0bf67c65dd4 to 29e2ce05c43552a7e383441624fa0cfdb840e786

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

 ​29e2ce0 `Skip _test_construction for the remaining test failures`

### comment:122 Changed 15 months ago by mkoeppe

• Authors changed from Simon King, Matthias Koeppe, Marc Mezzarobba to Simon King, Matthias Koeppe, Marc Mezzarobba, Travis Scrimshaw
• Status changed from needs_work to needs_review

Proposing to take care of the remaining failures in a follow-up ticket.

### comment:123 follow-ups: ↓ 124 ↓ 125 Changed 15 months ago by tscrim

• Reviewers set to Travis Scrimshaw

I am happy with that and the rest of the changes. Anyone else have any objections?

### comment:124 in reply to: ↑ 123 Changed 15 months ago by mmezzarobba

I am happy with that and the rest of the changes. Anyone else have any objections?

No, I was going to make the same suggestion as Matthias.

### comment:125 in reply to: ↑ 123 Changed 15 months ago by mkoeppe

• Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Matthias Koeppe
• Status changed from needs_review to positive_review

I am happy with that and the rest of the changes.

For completeness, I have reviewed your changes to the Zinbiel construction functor.

### comment:126 Changed 15 months ago by mkoeppe

Follow-up in #30574

### comment:127 Changed 15 months ago by tscrim

Is #30507 really a dependency? If not, we should remove it so this gets picked up into Sage.

### comment:128 Changed 15 months ago by mkoeppe

• Dependencies #30507 deleted

It isn't - thanks for catching this

### comment:129 Changed 15 months ago by vbraun

• Branch changed from public/ticket/15223 to 29e2ce05c43552a7e383441624fa0cfdb840e786
• Resolution set to fixed
• Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.