-
-
Notifications
You must be signed in to change notification settings - Fork 596
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
move invariant symmetric bilinear form to instance method #39672
base: develop
Are you sure you want to change the base?
move invariant symmetric bilinear form to instance method #39672
Conversation
Documentation preview for this PR (built with commit 7b9cab2; changes) is ready! 🎉 |
There is probably a better way to get the representation matrix than instantiating a Specht module within SymmetricGroupRepresentation_generic_class, given that The issue is that if you try to use Perhaps checking |
c9cf70e
to
11f79bd
Compare
I think GAP can find invariant bilinear forms. See Other comments:
|
It appears so. I am aware of the MeatAxe algorithm, and I believe this came up in discussion during #39200, #38455, and #38456. For the extended Cholesky decomposition, we did use the GAP forms package initially via If it's possible and better to use GAP, without the issues we ran into before, I am open to that.
True. I thought the rest of the docs made it clear the solution is a vector space of some dimension. I've changed "the" to "an".
Great point, thank you. I'll make that change and ensure tests are passing. |
@DaveWitteMorris Replaced I tried using a minimal set of generators (an |
The computation of G-invariant symmetric bilinear forms only depends on the group and representation. It is a general computation. Since there doesn't seem to be a generic class for finite group representations, for now we leave this in the symmetric group representation generic class. Now it can be tested, optimized, and possibly moved in the future. use self._ring and self._partition for self._specht to define self._specht in SymmetricGroupRepresentations_generic_class, _partition defined in init along with _ring, so we can call those via self. define self._specht to compute rho self._spect is only defined for unitary representations self._specht is defined within UnitaryRepresentation. here, we should be able to just get the representation matrix directly, and then we need the dimension. this can be computed as the trace of the identity usually, but since we're over a finite field, we should take the number of columns. better comments include 'symmetric' not all invariant bilinear forms are symmetric. these are, so we really need to specify that. add whitespace, check for zero dim'l nullspace update doc examples, remove whitespace compute U only sum over generators if a bilinear form is invariant under all generators, it is invariant for the entire group. replace "the" with "an" the solution is a vector space of some dimension, so the returned form is not the unique solution. even in the one dimensional case, it can be a scalar multiple.
411b430
to
d73f39c
Compare
I don't understand your code (including why you are using a sum). The way I look at it, the equation However, it is a basic principle of the sagemath project that we do not want to reinvent the wheel. (One reason for this is to avoid adding to the maintenance burden.) So I think you should have a good reason to code this, instead of just calling GAP. |
|
I think the bug needs to be fixed before the PR is merged, not after. I will post more comments later today when I have time. |
The point of this PR is to move this computation to a different part of the code, and abstract and streamline the code in the |
I think it's a good idea to have a method to find invariant bilinear forms for general representations, but the developer guide makes it clear that contributions to sagemath need to be high quality. I get the impression that you want to merge a half-baked PR and fix the code later, but that's not the way this project does things. a. When you wrote "Now it can be tested, optimized, and possibly moved in the future", I thought you meant while it is in draft status. Testing and moving (and obvious optimizations) are to be done before the code is merged. In particular, sagemath's deprecation policy makes it cumbersone to move (or rename) methods after they are part of the codebase. The algorithm works for (finite or infinite) semigroups, not just groups, so the method may belong somewhere near the top of b. The documentation of the method is wrong, so needs to be fixed. For example: The first line of the docstring says the method will return a symmetric form, but there is no guarantee that the form is symmetric. The method returns a basis of the space of invariant forms, not "a form". Is it really true that the space of invariant forms will generically be one-dimensional? Is it really true that "the forms decompose into a sum of symmetric and alternating"? It's not obvious to me in characteristic two. I also notice that a comment says "#solve the linear system for U for all group elements", even though it's only for the generators, not all group elements. c. The name d. The method sometimes returns a form and sometimes returns a list. It is much better to have a consistent return type. I think it would make sense to return a basis, so the return value would be an empty list when there are no invariant forms (instead of raising an exception). e. When I suggested getting rid of half of the variables, you responded that it would ignore the alternating forms. Well, since the name of the method is However, now that you bring it up, I certainly agree that the method should find both types. There should be a keyword argument to let the user choose which type they want (and something like f. I don't agree that implementing a libgap interface would be "reinventing the wheel". (It is eliminating code duplication, which is something quite different.) Although I think libgap should have been used from the start, you are right that the calculus has changed now that there is already code in sagemath. (However, I don't think it is accurate to call it "working" code, since there is a bug.) It would be reasonable to merge a corrected version of your code into sagemath, but then I think an issue should be opened to replace it with a libgap wrapper. g. I do not have time to write a libgap wrapper right now. When I saw the issue, I just thought I should make a few quick comments because I could see that you were headed in some wrong directions. I'm glad you have accepted some of my suggestions, but I have already spent a lot more time on this than I intended. h. I think additional comments would make the code easier to understand. When I first looked at it, I thought the i. The code needs to work for all (semi)groups, so there will not be any control at all over the generating set. I am glad you agreed to try to fix this. The PR can't get a positive review until the code is working properly. |
@DaveWitteMorris Before going on, I need to clarify something: do you understand that this code, aside from being moved to a class function, has already been merged in #38455? You're saying some conflicting things:
Further, you have stated that
Apologies, but this PR is simply meant to reorganize this code, not entirely refactor it. |
By moving this code, @jacksonwalters you are making it more general-purpose, so it should work in those cases. I also agree that the return time should be consistent as the user ahead of time will not know the dimension of the invariant forms ahead of time. I agree with most of @DaveWitteMorris's suggestions, although we should be careful to abide by the "the perfect is the enemy of the good" mantra. Let's try to spend a bit of time to optimize this now and fix any bugs we do come across. Solving Note also you do not need to create another representation. You already have |
@jacksonwalters: In answer to your question: no, I do not agree that this code has already been reviewed and merged. Most of my comments concern problems that arose in converting it to a class function. I do make mistakes when reviewing PR's, but I think all of my comments this time are valid, except perhaps the suggestion to use libgap, which you have already rejected, and the two that I phrased as questions (but now I'm pretty sure that my concern about characteristic two is correct). (However, I think something like "both" would be better than Also, I don't think my comments ask you to "entirely refactor" the code (unless you are referring to using libgap, which is not really relevant to my latest comments). They seem to me to be pretty typical (and pretty minor) reviewer comments. The biggest one is probably the suggestion to treat symmetric forms and alternating forms separately, but that should just be a few additional lines of code. (If it turns out to be a big job, it might be ok to leave it to a follow-up PR.) I apologize if "half-baked" was a poor choice of words, but it seems clear to me that the code is not ready to be merged, for the reasons I have listed, and my understanding was that you thought it was fine to have problems like this (even known bugs!) in code that is merged. It's not, and reviewers are supposed to point out these problems (and possible problems). |
I should also have said that it's fine if you disagree with some specific reviewer comments. Just give a good reason why. |
@tscrim Yes, the goal of this PR is to make it more general purpose. My idea was to do it in stages: 1) move this out of UnitaryRepresentations to general SymmetricGroupRepresentation so it at least has doctests 2) actually allow for the case of a higher dim'l solution space (we were only returning nullspace.basis()[0] before) 3) find out what needs to happen to move this all the way out to finite group representations generally (which there doesn't seem to be an obvious general class for.
With respect to bugs, this only happens if you replace Reducing to
If I try to call
@DaveWitteMorris I think we are (hopefully) in agreement - this current PR of course has not been merged, and I'll agree that more work is necessary if it is to be separated from
We had very lengthy discussions about using
No worries. I also apologize if I was being curt. I also want this code to be as good as it can possibly, and I'm willing to streamline and improve it. I agree that the code in this PR is not where it should be, but I maintain that the code we did merge in was thoroughly tested and as good as it could be for the cases we were interested in. Now that we're generalizing, I think it can be a lot better. I'll try to address the issue you mentioned and I concur with Travis that they're good observations. |
I think it makes more sense to move this to the |
if we move this up to `SymmetricGroupRepresentation_generic_class`, then we have to be careful about calling `representation_matrix`. For instance, if we create a `UnitaryRepresentation`, then trying to compute an `invariant_symmetric_bilinear_form` will call `representation_matrix`, which will then call `invariant_symmetric_bilinear_form`. In other words, `representation_matrix` needs to be from a Specht module. It makes more sense to move this code to `SpechtRepresentation` because for these invariant forms, we are always working with respect to a representation. It's probably possible to genrealize this beyond SymmetricGroupRepresentations, or use GAP, but for now this makes the most sense. We still create `self._specht` in `UnitaryRepresentation`, now can just call `invariant_symmetric_bilinear_form()`
note that [1,2] and [1,2,3,4,5] are both the identity permutation. need to be sure to use [2,1] for the '(1,2)' transposition and [2,3,4,5,1] for the `n`-cycle '(1,2,3,4,5)'.
@DaveWitteMorris I've resolved the bug with the small generating set. It was just that It makes sense, too, because the basis was just every elementary matrix since there were no conditions to be satisfied. |
@DaveWitteMorris @tscrim Here is an updated version using a
|
the solution space for reducible representations can easily be as big as the number of irreducible subrepresentations, or even bigger. so, "typically 1-dimensional" isn't really correct without extra assumptions |
Specht modules are irreducible in characteristic Is it true that there are no alternating forms for the symmetric group? Soon there'll be code for this, but it would be good to know some cases to expect. |
Yes, Specht modules are irreducible, in char 0 at least, but what does this have to do with the general functionality this PR is implementing? Yes, all the representations of the symmetric groups are real, so no alt. forms will arise in this case. |
When you say representations of the symmetric group are real, you mean defined over I'm viewing this PR as a half-step on the way to generalizing this for any representation of a finite group. For now, I'd just like this to be a method with appropriate doctests (noting that without this PR, it doesn't ever return more than one linearly independent matrix). This PR will include at least two optimizations now (using a 2-element generating set, cutting the number of variables in half by imposing the symmetry condition). Currently moving the code to Specht is causing issues. |
I was talking about char 0 case. I don't know much about the modular case. @darijgr is an expert on all things |
Sorry for not reading the whole thread, but here are a few things I do know about symmetric groups: For any Specht module S^D over any commutative ring (with D being any diagram -- not necessarily a partition shape), there is a canonical S_n-invariant symmetric bilinear form on S^D obtained by restricting the obvious form on the Young module M^D (the one which makes all tabloids orthonormal) to the submodule S^D. When the ring is a field of characteristic 0, this form is nondegenerate. To see this, it suffices to show it when the field is QQ (after all, this is a question of a determinant being nonzero), and there we can use the positive definiteness of the form on M^D and the fact that restricting a positive definite form to a subspace yields a positive definite form again. On the other hand, if the field has characteristic p between 1 and n, then this form can have a nontrivial kernel, even when D is a partition shape. This kernel is used to construct the actual irreps (see §5 in Mark Wildon's https://www.ma.rhul.ac.uk/~uvah099/Maths/Sym/SymGroup2014.pdf ). Over a field of characteristic 0, the actual partition-shaped Specht modules are irreducible, so the space of S_n-invariant bilinear forms is 1-dimensional, and thus all such forms are symmetric because the canonical one is. Over other fields or for other shapes, this might not be true. I definitely see how it's false for non-partition shapes (e.g., because of irreps occurring with multiplicities), but I'm less definite about the partition-shaped char-p case. Might be a good question. Corollary 13.17 in James's "The Representation Theory of the Symmetric Groups" might be useful. |
The computation of G-invariant symmetric bilinear forms only depends on the group and representation. Since there doesn't seem to be a generic class for finite group representations, for now we leave this in
SymmetricGroupRepresentation_generic_class
. Now it can be tested, optimized, and possibly moved in the future.📝 Checklist
⌛ Dependencies
#39200