Make DirichletGroup a new-style parent

[pjbruin]

The purpose of this ticket is to upgrade DirichletGroup from an old-style ParentWithMultiplicativeAbelianGens to a proper Parent.

At the same time, we place DirichletGroup in the category of Abelian groups.

Component: number theory

Keywords: Dirichlet group

Author: Peter Bruin

Branch/Commit: 3a2a89b

Reviewer: Jeroen Demeyer

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

[pjbruin]

Commit: 5c32c32

[pjbruin]

Branch: u/pbruin/18540-DirichletGroup_Parent

[pjbruin]

Work Issues: unpickling of old instances

[sagetrac-git]

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

1285b0e

Trac 18540: implement `__setstate__` to fix unpickling of old instances

[sagetrac-git]

Changed commit from 5c32c32 to 1285b0e

[pjbruin]

Changed work issues from unpickling of old instances to none

[tscrim]

comment:5

A couple of comments:

• Is Groups().Commutative() the best category? The list function seems like they should be in finite groups and that they are is finitely generated. I would say the category should be Groups().Commutative().Finite().FinitelyGenerated() (or at least a join with EnumeratedSets).

• While I doubt it was used, the __cmp__ did put a total ordering on the parents which I think we should keep by implementing a __lt__ (and using @total_ordering).

Looks good otherwise.

[sagetrac-git]

Changed commit from 1285b0e to b92b5b0

[sagetrac-git]

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

b92b5b0

Trac 18540: refine category of Dirichlet group if possible

[pjbruin]

comment:7

Replying to @tscrim:

A couple of comments:

• Is Groups().Commutative() the best category? The list function seems like they should be in finite groups and that they are is finitely generated. I would say the category should be Groups().Commutative().Finite().FinitelyGenerated() (or at least a join with EnumeratedSets).

Done, but only if we know that the group of n-th roots of unity in the base ring is finite. This is currently still the case for all Dirichlet groups that we can construct, but in the future we may admit base rings where this is not the case (the profinite completion of Z, for example).

• While I doubt it was used, the __cmp__ did put a total ordering on the parents which I think we should keep by implementing a __lt__ (and using @total_ordering).

I don't like this very much. First, the previous implementation actually does not make sense since it depends on a total ordering of all rings. Second, I cannot imagine why one would like to totally order all Dirichlet groups over all rings.

[jdemeyer]

Reviewer: Jeroen Demeyer

[jdemeyer]

comment:8

1. In the docstring of DirichletGroup_class, could you replace

Group of Dirichlet characters modulo `N` over a ring `R`.

by

Group of Dirichlet characters modulo `N` with values in a ring `R`.

(or something similar)

2. In the check

if isinstance(x, (int, rings.Integer)) and x == 1:

I think it is better to remove isinstance(x, (int, rings.Integer)) and allow any kind of 1 (but then you will probably need to add a try/except in case that x == 1 fails).

[jdemeyer]

comment:9

I have no further comments. If you make the above changes and doctests still pass, you can set this to positive_review.

[jdemeyer]

comment:10

Additional remark: it would be good to fix these pyflakes warnings:

$ pyflakes src/sage/modular/dirichlet.py
src/sage/modular/dirichlet.py:550: 'latex' imported but unused
src/sage/modular/dirichlet.py:800: local variable 'R' is assigned to but never used
src/sage/modular/dirichlet.py:1691: local variable 'R' is assigned to but never used
src/sage/modular/dirichlet.py:2312: local variable 'n' is assigned to but never used
src/sage/modular/dirichlet.py:2314: local variable 'p' is assigned to but never used
src/sage/modular/dirichlet.py:2376: local variable 'one' is assigned to but never used
src/sage/modular/dirichlet.py:2377: local variable 'zeta' is assigned to but never used

[sagetrac-git]

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

3a2a89b

Trac 18540: reviewer comments

[sagetrac-git]

Changed commit from b92b5b0 to 3a2a89b

[vbraun]

Changed branch from u/pbruin/18540-DirichletGroup_Parent to 3a2a89b