Opened 3 years ago
Closed 2 years ago
#22913 closed enhancement (fixed)
Reynolds Operator
Reported by:  rlmiller  Owned by:  

Priority:  minor  Milestone:  sage8.0 
Component:  group theory  Keywords:  
Cc:  bhutz  Merged in:  
Authors:  Rebecca Lauren Miller  Reviewers:  Ben Hutz, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  caaa7ee (Commits)  Commit:  caaa7eecb87369a84f41e97c477ccf27d4e01bff 
Dependencies:  Stopgaps: 
Description
Compute the Reynolds Operator for finite groups G.
Change History (25)
comment:1 Changed 3 years ago by
 Branch set to u/rlmiller/reynolds
comment:2 Changed 3 years ago by
 Cc Ben Hutz added
 Commit set to f5f015034b44e593ac0033d87ab707e9ea379bf4
comment:3 Changed 3 years ago by
 Commit changed from f5f015034b44e593ac0033d87ab707e9ea379bf4 to c7d4c767243b9b86a3eb86a12745fba20b8b15af
Branch pushed to git repo; I updated commit sha1. New commits:
c7d4c76  22913 fixed merge conflict

comment:5 Changed 3 years ago by
 Branch changed from u/rlmiller/reynolds to u/bhutz/reynolds
comment:6 Changed 3 years ago by
 Commit changed from c7d4c767243b9b86a3eb86a12745fba20b8b15af to bc9096e8712afa1347319c16513944a792ee1656
Apparently, my comment from a couple days ago failed to submit. Let me try again.
The code changes are just some minor cleanup, but you need to think about what to do with base fields. For example, the 4th character in the tetrahedral group has an invariant defined over QQ(sqrt(3)) (or maybe QQ(sqrt(3))). It might be nice if the following gave back the invariant in that field. At the moment this doesn't even run since you only take into account the group base field and the character base field.
K.<i> = CyclotomicField(4) Tetra = MatrixGroup([(1+i)/2,(1+i)/2, (1+i)/2,(1i)/2], [0,i, i,0]) xi = Tetra.character(Tetra.character_table()[4]) L.<w>=QuadraticField(3) Tetra.reynolds_operator(x^4, xi)
This example is a good one to figure out what kind of user experience you want for base fields.
New commits:
bc9096e  22913: some code cleanup

comment:7 Changed 3 years ago by
 Branch changed from u/bhutz/reynolds to u/rlmiller/reynolds
comment:8 Changed 3 years ago by
 Commit changed from bc9096e8712afa1347319c16513944a792ee1656 to 49873bbb4bd2b23eb262d62621d55f12f94a7445
 Status changed from new to needs_review
New commits:
49873bb  22913 user can now get smallest field for invariant, foxed typos, added example

comment:9 Changed 3 years ago by
 Branch changed from u/rlmiller/reynolds to u/bhutz/reynolds
comment:10 Changed 3 years ago by
 Commit changed from 49873bbb4bd2b23eb262d62621d55f12f94a7445 to 465928a6263ff3f2b833f2244561c4adc63bb3d7
 Reviewers set to Ben Hutz
 Status changed from needs_review to needs_work
I think that is better with the fields, although you missed the case where R.degree()==1
K.<i> = CyclotomicField(4) Tetra = MatrixGroup([(1+i)/2,(1+i)/2, (1+i)/2,(1i)/2], [0,i, i,0]) xi = Tetra.character(Tetra.character_table()[4]) L.<v> = QuadraticField(5) R.<x,y> = QQ[] f=x^4 g=Tetra.reynolds_operator(f,xi) g
Also relative extensions do not work (you can decide if you'll require absolute fields or deal with relative ones)
K.<i> = CyclotomicField(4) G = MatrixGroup([(1+i)/2,(1+i)/2, (1+i)/2,(1i)/2], [0,i, i,0]) xi = G.character(Tetra.character_table()[4]) L.<v> = QuadraticField(5) R.<x>=L[] LL.<w> = L.extension(x^2+v) R.<x,y> = LL[] f=x^4 g=Tetra.reynolds_operator(f,xi) g
QQbar doesn't work either and it seems like that should probably work
K.<i> = CyclotomicField(4) G = MatrixGroup([(1+i)/2,(1+i)/2, (1+i)/2,(1i)/2], [0,i, i,0]) xi = G.character(Tetra.character_table()[4]) L.<v> = QuadraticField(5) R.<x>=L[] LL.<w> = L.extension(x^2+v) R.<x,y> = QQbar[] f=x^4 g=Tetra.reynolds_operator(f,xi) g
It would be good to give a better error when the dimension of the polynomial ring does not match the dimension of the matrix group
G = MatrixGroup(DihedralGroup(4)) xi = G.character(G.character_table()[1]) R.<x,y>=QQ[] f=x^4 g=G.reynolds_operator(f,xi) g
Otherwise, everything else I tried worked just fine.
New commits:
465928a  22913: minor code clean up

comment:11 Changed 3 years ago by
 Branch changed from u/bhutz/reynolds to u/rlmiller/reynolds
comment:12 Changed 3 years ago by
 Commit changed from 465928a6263ff3f2b833f2244561c4adc63bb3d7 to 3a7884fe2ab833d4213e161fd5ddc33b6a60b024
 Status changed from needs_work to needs_review
New commits:
3a7884f  22913 fixed over qqbar, algebraic fields, and number fields

comment:13 Changed 3 years ago by
 Branch changed from u/rlmiller/reynolds to u/bhutz/reynolds
comment:14 Changed 3 years ago by
 Commit changed from 3a7884fe2ab833d4213e161fd5ddc33b6a60b024 to 0982b16020e971c12f57c6dbc9214092f43686e6
 Status changed from needs_review to needs_work
I reduced the many nested if/then/else into something I think is more readable. I also tried to do some optimization of doctesting. Even creating some of these groups is slow, so I combined many tests. I also limited it to one test per case and limited the Tetrahedral use as it is very slow to create. I think I reduced the total test time by about 8 seconds.
I also noticed the even in the trivial character case you may need to extend. The following then fails.
sage: K.<i> = CyclotomicField(4) sage: Tetra = MatrixGroup([(1+i)/2,(1+i)/2, (1+i)/2,(1i)/2], [0,i, i,0]) sage: L.<v> = QuadraticField(5) sage: R.<x,y> = L[] sage: Tetra.reynolds_operator(x^4)
All other functionality tested was good.
New commits:
0982b16  22913: some code and doctest optimization

comment:15 Changed 2 years ago by
 Branch changed from u/bhutz/reynolds to u/rlmiller/reynolds
comment:16 Changed 2 years ago by
 Commit changed from 0982b16020e971c12f57c6dbc9214092f43686e6 to 6434a9e3a37559134f3cd058ff82e22b16b105ff
 Status changed from needs_work to needs_review
Fixed trivial character case and added the example above.
New commits:
6434a9e  22913 fixed error for when chi is none

comment:17 Changed 2 years ago by
A note on the doc: you should have a short 1line description and then the full body. So something like
 Compute the Reynolds Operator of this finite group `G`. This is the  projection from the polynomial ring to the ring of relative invariants. + Compute the Reynolds operator of ``self``. + + Let `G` be a finite group. The Reynolds operator of `G` is the + projection from the polynomial ring to the ring of relative invariants.
comment:18 Changed 2 years ago by
 Branch changed from u/rlmiller/reynolds to u/bhutz/reynolds
comment:19 Changed 2 years ago by
 Commit changed from 6434a9e3a37559134f3cd058ff82e22b16b105ff to 2c31d35ca7965ec0bf52cd03d65ed9949371f7d9
Branch pushed to git repo; I updated commit sha1. New commits:
2c31d35  22913: fix parameter spacing in molien series

comment:20 Changed 2 years ago by
same basic changes as last time. I changed how the composition was generated to be more readable. I also adjusted the error messages and improved the handling in the characteristic p>0 trivial character case.
Also, the one line description was fixed.
I think this works in all cases it is reasonable to make it work for at this time. So if you test it again and are fine with my changes, I'm ok with marking it positive.
comment:21 Changed 2 years ago by
I have one more comment and some additional nitpicks.
Since you are changing the argument name for molien_series
, you should deprecate the old one in case anyone has explicitly done molien_series(xi=foo)
. The best way to do that is to use
from sage.misc.decorators import rename_keyword @rename_keyword(22913, xi='chi') def molien_series(self, chi=None, return_series=True, prec=20, variable='t'):
Nitpicks:
 def reynolds_operator(self, poly, chi = None): + def reynolds_operator(self, poly, chi=None):
 relative invariants. [Stu1993]_. If possible, the invariant is + relative invariants [Stu1993]_. If possible, the invariant is
  ``poly``  a polynomial from K[x] +  ``poly``  a polynomial
or a polynomial in `K[x]`
AUTHORS:  Rebecca Lauren Miller and Ben Hutz + Rebecca Lauren Miller and Ben Hutz
poly_gens = vector(poly.parent().gens())  F = L(0) + F = L.zero() for g in self:  F += L(chi(g))*poly(*g.matrix().change_ring(L)*poly_gens) + F += L(chi(g)) * poly(*g.matrix().change_ring(L)*poly_gens) F /= self.order()
comment:22 Changed 2 years ago by
 Commit changed from 2c31d35ca7965ec0bf52cd03d65ed9949371f7d9 to caaa7eecb87369a84f41e97c477ccf27d4e01bff
Branch pushed to git repo; I updated commit sha1. New commits:
caaa7ee  22913: minor fixes

comment:23 Changed 2 years ago by
I made all the changes except the deprecation. The parameter for molien_series() was introduced in #22835, so has never been part of a release version of sage. Just want to confirm that the really needs a deprecation.
comment:24 Changed 2 years ago by
 Reviewers changed from Ben Hutz to Ben Hutz, Travis Scrimshaw
 Status changed from needs_review to positive_review
There is some slight ambiguity on that, so I don't think we have definite rule. However, I agree with all other changes. Thanks.
comment:25 Changed 2 years ago by
 Branch changed from u/bhutz/reynolds to caaa7eecb87369a84f41e97c477ccf27d4e01bff
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Merge commit '81dcaba8f63d5ba43f52d80f20474d7859806ee2' of git://trac.sagemath.org/sage
22913 Implimenting Reynold's Operator