-
-
Notifications
You must be signed in to change notification settings - Fork 438
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
Add minimal_normal_subgroups and maximal_normal_subgroups functions #37038
Add minimal_normal_subgroups and maximal_normal_subgroups functions #37038
Conversation
This commit changes the _algebraic_ function of Expression class to simplify the expression with simplify_full() function before converting it to algebraic number.
Added the doctest for _element_constructor_() function of AlgebraicField class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The result should be converted back into Sage (permutation) (sub)groups.
Also, I don't see why
I have done some necessary changes. But I just found that these methods can be implemented for all groups as the functions MaximalNormalSubgroups and MinimalNormalSubgroups works for all groups in gap, so I am considering implementing this in Sage for all groups, but I am having difficulty in finding the appropriate location to define these functions. Can you help me with this? |
You can add it to the |
I have relocated the function, but I believe the implementation is suboptimal. This is because it will consume a significant amount of time, as it needs to generate the generators first and then convert them into a subgroup (as demonstrated below). I attempted to explore alternative options but was unsuccessful. Do you have any suggestions for the fix for this? sage/src/sage/groups/libgap_wrapper.pyx Line 359 in 297e8b2
Also, were you referring to this particular Class? Because now for permutation groups it is giving an AttributeError (seems like permutationgroups are not derived from this class). |
Within |
I have implemented the modifications; however, I remain uncertain about the location of these functions. I am unable to invoke these functions when I have a permutation group(It is giving me AttributeError). Because this class is never inherited in the PermutationGroup class. So what should I do ? Because I am not sure whether inheriting that function in PermutationGroup class will be good or not. |
Something that is annoying is that permutation groups don't seem to use libgap. I think you need to have your previous implement in |
Please also squash and give at least a little bit better commit messages for the first two. These automated ones from github are not good (I want to fire that "feature" into the sun). |
1e10cc6
to
1d90107
Compare
ce236ee
to
0f17e65
Compare
Done! I have applied all the changes that you suggested. Thank you for your help, @tscrim throughout :) . I learned a lot while working on this, and I won't make these types of mistakes again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem. Thank you for the changes. This is just dealing with some trickier parts of Sage with how things are implemented because of the interfaces.
I am cc-ing @dimpase in case he has some comments.
@fchapoton did some work on removing pexpect GAP and replacing it with libgap just last week. Maybe it should be taken into account here |
Good point. I rebased this off the latest develop, which should have #36889 included. Since there are no merge conflicts and @RuchitJagodara wrote the code with future-proofing in mind, there shouldn’t be any issues (which will be reflected in the tests all passing). I also don’t see any way to make additional improvements at this point either. |
Documentation preview for this PR (built with commit fa26be9; changes) is ready! 🎉 |
…groups functions for permutation groups - Added the function This patch implements a minimal_normal_subgroups function in PermutationGroup_generic class. <!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes sagemath#1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have created tests covering the changes. - [x] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> NA URL: sagemath#37038 Reported by: Ruchit Jagodara Reviewer(s): Ruchit Jagodara, Travis Scrimshaw
…groups functions for permutation groups - Added the function This patch implements a minimal_normal_subgroups function in PermutationGroup_generic class. <!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes sagemath#1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have created tests covering the changes. - [x] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> NA URL: sagemath#37038 Reported by: Ruchit Jagodara Reviewer(s): Ruchit Jagodara, Travis Scrimshaw
…groups functions for permutation groups - Added the function This patch implements a minimal_normal_subgroups function in PermutationGroup_generic class. <!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes sagemath#1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have created tests covering the changes. - [x] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> NA URL: sagemath#37038 Reported by: Ruchit Jagodara Reviewer(s): Ruchit Jagodara, Travis Scrimshaw
This patch implements a minimal_normal_subgroups function in PermutationGroup_generic class.
📝 Checklist
⌛ Dependencies
NA