Opened 3 years ago

Closed 2 years ago

#22913 closed enhancement (fixed)

Reynolds Operator

Reported by: rlmiller Owned by:
Priority: minor Milestone: sage-8.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 rlmiller

  • Branch set to u/rlmiller/reynolds

comment:2 Changed 3 years ago by rlmiller

  • Cc Ben Hutz added
  • Commit set to f5f015034b44e593ac0033d87ab707e9ea379bf4

New commits:

a04a166Merge commit '81dcaba8f63d5ba43f52d80f20474d7859806ee2' of git://trac.sagemath.org/sage
f5f015022913 Implimenting Reynold's Operator

comment:3 Changed 3 years ago by git

  • Commit changed from f5f015034b44e593ac0033d87ab707e9ea379bf4 to c7d4c767243b9b86a3eb86a12745fba20b8b15af

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

c7d4c7622913 fixed merge conflict

comment:4 Changed 3 years ago by rlmiller

  • Cc bhutz added; Ben Hutz removed

Fixed Merge Conflict

comment:5 Changed 3 years ago by bhutz

  • Branch changed from u/rlmiller/reynolds to u/bhutz/reynolds

comment:6 Changed 3 years ago by bhutz

  • 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 clean-up, 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,(-1-i)/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:

bc9096e22913: some code clean-up

comment:7 Changed 3 years ago by rlmiller

  • Branch changed from u/bhutz/reynolds to u/rlmiller/reynolds

comment:8 Changed 3 years ago by rlmiller

  • Commit changed from bc9096e8712afa1347319c16513944a792ee1656 to 49873bbb4bd2b23eb262d62621d55f12f94a7445
  • Status changed from new to needs_review

New commits:

49873bb22913 user can now get smallest field for invariant, foxed typos, added example

comment:9 Changed 3 years ago by bhutz

  • Branch changed from u/rlmiller/reynolds to u/bhutz/reynolds

comment:10 Changed 3 years ago by bhutz

  • 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,(-1-i)/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,(-1-i)/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,(-1-i)/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:

465928a22913: minor code clean up

comment:11 Changed 3 years ago by rlmiller

  • Branch changed from u/bhutz/reynolds to u/rlmiller/reynolds

comment:12 Changed 3 years ago by rlmiller

  • Commit changed from 465928a6263ff3f2b833f2244561c4adc63bb3d7 to 3a7884fe2ab833d4213e161fd5ddc33b6a60b024
  • Status changed from needs_work to needs_review

New commits:

3a7884f22913 fixed over qqbar, algebraic fields, and number fields

comment:13 Changed 3 years ago by bhutz

  • Branch changed from u/rlmiller/reynolds to u/bhutz/reynolds

comment:14 Changed 3 years ago by bhutz

  • 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 doc-testing. 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,(-1-i)/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:

0982b1622913: some code and doctest optimization

comment:15 Changed 2 years ago by rlmiller

  • Branch changed from u/bhutz/reynolds to u/rlmiller/reynolds

comment:16 Changed 2 years ago by rlmiller

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

6434a9e22913 fixed error for when chi is none

comment:17 Changed 2 years ago by tscrim

A note on the doc: you should have a short 1-line 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 bhutz

  • Branch changed from u/rlmiller/reynolds to u/bhutz/reynolds

comment:19 Changed 2 years ago by git

  • Commit changed from 6434a9e3a37559134f3cd058ff82e22b16b105ff to 2c31d35ca7965ec0bf52cd03d65ed9949371f7d9

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

2c31d3522913: fix parameter spacing in molien series

comment:20 Changed 2 years ago by bhutz

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 tscrim

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 git

  • Commit changed from 2c31d35ca7965ec0bf52cd03d65ed9949371f7d9 to caaa7eecb87369a84f41e97c477ccf27d4e01bff

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

caaa7ee22913: minor fixes

comment:23 Changed 2 years ago by bhutz

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 tscrim

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

  • Branch changed from u/bhutz/reynolds to caaa7eecb87369a84f41e97c477ccf27d4e01bff
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.