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
Semimonomial transformation groups code is sensitive to Permutations global options #15576
Comments
Branch: u/tfeulner/ticket/15576 |
Commit: |
I think there should be only this single occurrence of the New commits:
|
comment:3
Thanks for taking care of this. The dependency is gone indeed. Could you maybe also document the choice of multiplication order in the (module) documentation? And while at that:
either it should be "The semimonomial...", not "A semimonomial...", or it should be "a group of ring automorphisms", not "the group of ring automorphisms". In general, it should be impossible to compute the group of all automorphisms of a ring, so I suspect it's either "a group" or you are only considering finite fields? |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:5
Thanks for your comments. You are right, up to now it is only possible to construct the semimonomial group over a finite field. My plan for the future is to provide an optional package, which implements finite chain rings and semimonomial groups defined over them. While we are at it, the |
comment:6
Where does I prefer using In
you are using 0-based indexing of permutations; I'm not sure if this is what you want (it's doc, not code). You speak of the semimonomial group over a ring R of either "length n" (in the definition) or "degree n" (in other parts of the doc). I think it would help to settle upon one notation (or define them both). Other than this, the code looks fine. Thanks for the quick response, and set this to positive_review once the above issues are fixed! |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:8
Replying to @darijgr:
Well, what I wanted to say is, that we need two functions to implement the action of the permutation group on list/vectors of length The current implementation of Using
But this should become the topic of a separate ticket. Thanks for your careful reading, I think I have fixed them all. |
comment:9
Attention: branch change! I'm a bit surprised that you changed the definite articles back to the indefinites, so I'm suggesting to change them back in my commit. (Also, "the group of degree n over a ring R" sounds better than "the group over a ring R of degree n" to my ears.) If my edits are OK to you, please set this to positive_review. Thanks for your work! EDIT: I see what you mean by adding new |
comment:10
Trac is preventing me from changing the branch to public/ticket/15576, so maybe you can just merge this into your branch or write a big fat warning message on the ticket. |
Changed branch from u/tfeulner/ticket/15576 to public/ticket/15576 |
comment:14
Thanks for your help, Darij. All your changes look good to me. Of course, an experienced programmer could write these action methods very quickly. But, by allowing the user to freely decide on the multiplication rule, you also have to think about these dependencies and modifiy the existing code. |
comment:15
Please fill in the author/reviewer fields |
Author: Thomas Feulner |
Reviewer: Darij Grinberg |
As detailed in #14885, it is not healthy for code to rely on the
__mul__
operation on permutations, since this operation depends on thePermutations().global_options()['mul']
variable which can change at runtime. It is better to use theleft_action_product
andright_action_product
methods introduced in #15174 (formerly known as_left_to_right_multiply_on_left
and_left_to_right_multiply_on_right
).My tests show some dependence on the
__mul__
method insage/groups/semimonomial_transformations/semimonomial_transformation.pyx
andsage/groups/semimonomial_transformations/semimonomial_transformation_group.py
, although it might be that only one of these files depends on it and the other depends on the first file. Unfortunately I don't have time to study this in detail, as I'd first have to read up on the definitions.CC: @sagetrac-tfeulner
Component: group theory
Keywords: permutation, semimonomial transformation
Author: Thomas Feulner
Branch/Commit: public/ticket/15576 @
cbb5110
Reviewer: Darij Grinberg
Issue created by migration from https://trac.sagemath.org/ticket/15576
The text was updated successfully, but these errors were encountered: