simon-king-jena commented 11 years ago

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.

CC: @novoselt @nthiery @sagetrac-sage-combinat @tscrim @fchapoton @mwageringel @kwankyu @dkrenn

Component: coercion

Keywords: construction functor, test suite, sd53

Author: Simon King, Matthias Koeppe, Marc Mezzarobba, Travis Scrimshaw

Branch: 29e2ce0

Reviewer: Travis Scrimshaw, Matthias Koeppe

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

simon-king-jena commented 11 years ago

comment:1

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:

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.

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.

simon-king-jena commented 11 years ago

comment:2

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.

simon-king-jena commented 11 years ago

Branch: u/SimonKing/ticket/15223

simon-king-jena commented 11 years ago

comment:4

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:

Let the construction be None
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.

simon-king-jena commented 11 years ago

comment:5

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

simon-king-jena commented 11 years ago

comment:6

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.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 11 years ago

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

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 11 years ago

Commit: a81fcc1

simon-king-jena commented 11 years ago

comment:8

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.

simon-king-jena commented 11 years ago

comment:10

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?

nbruin commented 11 years ago

comment:11

Replying to @simon-king-jena:

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.

nthiery commented 11 years ago

comment:12

Replying to @simon-king-jena:

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

simon-king-jena commented 11 years ago

comment:13

Replying to @nbruin:

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

simon-king-jena commented 11 years ago

comment:14

Replying to @nthiery:

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.

simon-king-jena commented 11 years ago

comment:15

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.

a1031e5c-5e2c-4c90-8ba4-91be67e304df commented 11 years ago

comment:16

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.

simon-king-jena commented 11 years ago

comment:17

Replying to @sagetrac-jkeitel:

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.

nbruin commented 11 years ago

comment:18

Replying to @simon-king-jena:

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

nbruin commented 11 years ago

comment:19

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.

simon-king-jena commented 11 years ago

comment:20

Replying to @nbruin:

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.

simon-king-jena commented 11 years ago

comment:21

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:

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.
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.

simon-king-jena commented 11 years ago

comment:22

Replying to @simon-king-jena:

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...

simon-king-jena commented 11 years ago

comment:23

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"?

simon-king-jena commented 11 years ago

comment:24

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

one should not do primality test when creating the IntegerModRing
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
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.
Depending on your answers to my questions, I open another new ticket to address them.
I make these one or two tickets a dependency for this ticket.

simon-king-jena commented 11 years ago

Dependencies: #15229

simon-king-jena commented 11 years ago

comment:25

I have opened #15229 for the new problems.

simon-king-jena commented 11 years ago

Changed keywords from none to construction functor, test suite, sd53

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 11 years ago

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`

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 11 years ago

Changed commit from a81fcc1 to cb4929a

simon-king-jena commented 11 years ago

Author: Simon King

simon-king-jena commented 11 years ago

comment:28

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!

mezzarobba commented 10 years ago

comment:29

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

simon-king-jena commented 10 years ago

comment:30

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.

simon-king-jena commented 10 years ago

comment:31

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

mezzarobba commented 10 years ago

comment:32

Replying to @simon-king-jena:

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 item had failures:
   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.

mezzarobba commented 10 years ago

comment:33

Replying to @mezzarobba:

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

mezzarobba commented 10 years ago

comment:34

Replying to @mezzarobba:

I'm currently checking whether the problem exists on develop

Apparently it doesn't.

mezzarobba commented 10 years ago

comment:35

Replying to @mezzarobba:

Replying to @mezzarobba:

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?

simon-king-jena commented 10 years ago

comment:36

Replying to @mezzarobba:

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Changed commit from cb4929a to d0c42ff

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

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`

simon-king-jena commented 10 years ago

comment:38

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.

simon-king-jena commented 10 years ago

comment:39

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.

simon-king-jena commented 10 years ago

comment:40

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.

simon-king-jena commented 10 years ago

Work Issues: algebraic extension functor for residue fields

simon-king-jena commented 10 years ago

comment:41

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)

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Changed commit from d0c42ff to ab2c12e

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

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

ab2c12e Trac 15223: Construction of residue fields

simon-king-jena commented 10 years ago

comment:43

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

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

b23f18d Trac 15223: Construction of field extensions with implementation

sagemath / sage

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

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.