Skip to content
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 HallSubgroup fallback method #3080

Merged
merged 1 commit into from
Jan 9, 2019

Conversation

fingolfin
Copy link
Member

This allows computing Hall subgroups for arbitrary finite groups
via an isomorphism into a permutation group.

This allows computing Hall subgroups for arbitrary finite groups
via an isomorphism into a permutation group.
@fingolfin fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: library labels Dec 3, 2018
@fingolfin fingolfin requested a review from hulpke December 3, 2018 22:07
Copy link
Contributor

@hulpke hulpke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it is a fallback method, wouldn't it make sense to rank it with a value of -1, so that any other method for finite groups will automatically rank higher?

Similar issues are likely to hold for for other non-basic routines: Conjugacy classes, subgroups, automorphisms and so on. They work for permutation groups, if handled through nice monomorphism, or if FittingFreeLiftSetup is available, but there is no translation for other basic cases.

@fingolfin
Copy link
Member Author

I don't quite see why adjusting the rank by -1 would be useful, it seems rather arbitrary to me. After all, why would any future "generic" method for finite groups necessarily be better than this one? Instead, I'd think that any such method would need to be evaluated against the one added in this PR; and then the two should either be merged, using a heuristic to choose which approach to use; or if the new one is strictly superior to this method here, then the new should simply replace this one. And if a package wants to add a better method, they can adjust the rank by +1 instead.

(That said, I won't resist if people really think it's necessary to adjust the rank by -1, it just seems pointless from my vantage point).

As to other operations being affected: sure, you are of course absolutely right. See also issues #1580 and #596.

As discussed on PR #1580, instead of adding this method, we could certainly also try to give the user a more helpful method other than "method not found", something like "... perhaps try converting your group to another representation first". It just is note quite clear where to add that.

@hulpke
Copy link
Contributor

hulpke commented Dec 6, 2018

@fingolfin
Ranking at -1 would be an indication (both in looking at the code, and in execution preference) that this is a method which is there as convenience to users (to make the command work in every representation) but makes no claims of being effective. (For example, for matrix groups, if the matgrp package is loaded and FittingFreeLiftSetup has been called on the group, I would expect that method to be better.)
Of course, if you have examined the problem and feel confident in that this method is as good as generic can be, then add a comment about this and rank it at 0.

@markuspf markuspf merged commit 1625605 into gap-system:master Jan 9, 2019
@fingolfin fingolfin deleted the mh/HallSubgroup-for-finite branch January 9, 2019 16:20
@markusbaumeister markusbaumeister added the release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes label Apr 16, 2019
@DominikBernhardt DominikBernhardt added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 22, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: added PRs introducing changes that have since been mentioned in the release notes topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants