Opened 5 years ago
Closed 5 years ago
#17317 closed enhancement (fixed)
Add unit_group() method to IntegerModRing
Reported by:  pbruin  Owned by:  

Priority:  major  Milestone:  sage6.4 
Component:  number theory  Keywords:  unit group 
Cc:  rbeezer, fwclarke, kcrisman, wuthrich, jpflori  Merged in:  
Authors:  Peter Bruin  Reviewers:  Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  a861303 (Commits)  Commit:  a8613031ce7f122a91d2ca6257ee50d7bebc3f32 
Dependencies:  #17315  Stopgaps: 
Description (last modified by )
This ticket implements a unit_group()
method for Z/nZ. Example:
sage: A = Zmod(24) sage: G = A.unit_group(); G Multiplicative Abelian group isomorphic to C2 x C2 x C2 sage: G.gens_orders() (2, 2, 2) sage: G.gens_values() (7, 13, 17)
At the moment, there is no new class for such groups; it uses AbelianGroupWithValues
. However, this could easily be changed in the future if required.
The unit_group()
method admits an optional algorithm
argument (default: 'sage'
). This can be set to 'pari'
to use PARI's znstar()
function (see #17315). This gives different generators in general.
See also:
Change History (17)
comment:1 Changed 5 years ago by
 Branch set to u/pbruin/17317IntegerModRing_unit_group
 Commit set to 7f38953bdbc38709a218533934d759abc5325f76
 Status changed from new to needs_review
comment:2 Changed 5 years ago by
 Cc wuthrich added
comment:3 Changed 5 years ago by
 Status changed from needs_review to needs_work
comment:4 Changed 5 years ago by
I would also add doctests for the special cases Zmod(1)
and Zmod(2)
comment:5 followup: ↓ 11 Changed 5 years ago by
If possible, I would like to see the changes to the Dirichlet character stuff in a different ticket (to make reviewing easier).
comment:6 Changed 5 years ago by
Why not
from sage.rings.arith import primitive_root def _unit_gens_primecase(p): return primitive_root(p, check=False) def _unit_gens_primepowercase(p, r): return primitive_root(p**r, check=False)
comment:7 followup: ↓ 12 Changed 5 years ago by
For number fields, we have
sage: QuadraticField(5).unit_group(5) Unit group with structure C2 x Z of Number Field in a with defining polynomial x^2  5 sage: QuadraticField(5).unit_group(5).number_field() Number Field in a with defining polynomial x^2  5
But in the current case:
sage: Zmod(1000).unit_group() Multiplicative Abelian group isomorphic to C2 x C2 x C100
Though we can see which ring we're working with via
sage: Zmod(1000).unit_group().values_group() Ring of integers modulo 1000
this terminology is very nonintuitive. I think it would be worth having a class for these groups, with a ring
method and a more complete _repr_
.
comment:8 Changed 5 years ago by
 Description modified (diff)
comment:9 Changed 5 years ago by
 Commit changed from 7f38953bdbc38709a218533934d759abc5325f76 to a6e38c686b028bbbbd112fdeb569e09dd932e2a5
Branch pushed to git repo; I updated commit sha1. New commits:
3597314  Trac 17317: revert some of the changes in the previous commit

758798e  Trac 17317: rewrite _unit_gens_primepowercase() using primitive_root()

7f45af0  Trac 17317: slightly change error message and add doctests

a6e38c6  Trac 17317: temporary special case n = 1 to avoid having to fix many doctests

comment:10 Changed 5 years ago by
 Commit changed from a6e38c686b028bbbbd112fdeb569e09dd932e2a5 to 7e046ddd5ddf66209daf7dd4755e76e4dbbedf56
Branch pushed to git repo; I updated commit sha1. New commits:
7e046dd  Trac 17317: add optional keywords to unit_gens() and update related documentation

comment:11 in reply to: ↑ 5 ; followup: ↓ 13 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:12 in reply to: ↑ 7 Changed 5 years ago by
Replying to fwclarke:
For number fields, we have
sage: QuadraticField(5).unit_group(5) Unit group with structure C2 x Z of Number Field in a with defining polynomial x^2  5 sage: QuadraticField(5).unit_group(5).number_field() Number Field in a with defining polynomial x^2  5But in the current case:
sage: Zmod(1000).unit_group() Multiplicative Abelian group isomorphic to C2 x C2 x C100Though we can see which ring we're working with via
sage: Zmod(1000).unit_group().values_group() Ring of integers modulo 1000this terminology is very nonintuitive. I think it would be worth having a class for these groups, with a
ring
method and a more complete_repr_
.
I agree, but this should probably be done on a different ticket. I think it won't cause problems if we change the precise type of group output by unit_group()
in the future. (Maybe we could just make it a subclass of AbelianGroupWithValues
.)
comment:13 in reply to: ↑ 11 Changed 5 years ago by
 Reviewers set to Jeroen Demeyer
Replying to pbruin:
I think the new commits address your other comments, back to
needs_review
.
From a quick look through the diffs, this is indeed the case. I'll make a deeper review later.
comment:14 Changed 5 years ago by
 Cc jpflori added
comment:15 Changed 5 years ago by
 Branch changed from u/pbruin/17317IntegerModRing_unit_group to u/jdemeyer/ticket/17317
 Created changed from 11/12/14 00:16:58 to 11/12/14 00:16:58
 Modified changed from 11/17/14 09:32:58 to 11/17/14 09:32:58
comment:16 Changed 5 years ago by
 Commit changed from 7e046ddd5ddf66209daf7dd4755e76e4dbbedf56 to a8613031ce7f122a91d2ca6257ee50d7bebc3f32
 Status changed from needs_review to positive_review
I added a trivial patch, looks good!
New commits:
a861303  Trac 17317: Factor out computation of p**r

comment:17 Changed 5 years ago by
 Branch changed from u/jdemeyer/ticket/17317 to a8613031ce7f122a91d2ca6257ee50d7bebc3f32
 Resolution set to fixed
 Status changed from positive_review to closed
First impressions:
algorithm="sage"
andalgorithm="pari"
can be. I would addZmod(319)
as example. Also a larger example, sayZmod(lcm([1..40]))
or so...**kwds
argument tounit_gens()
:repr(algorithm)
in the error message, something like:This error should also be doctested in a
TESTS::
block.