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

Fix SubgroupsSolvableGroup to honor retnorm option even for trivial groups #4855

Merged
merged 1 commit into from
May 11, 2022

Conversation

RamonEstebanRomero
Copy link
Contributor

@RamonEstebanRomero RamonEstebanRomero commented Apr 7, 2022

The function SubgroupsSolvableGroup with a trivial group with an argument does not return the list of normalizers when called with the option retnorm. I suggest considering this flag for trivial groups. I propose moving first the parsing of the retnorm flag and immediately checking the trivial case. Please check I have not destroyed the rest of the function!

Text for release notes

See title (or leave it up to you)

@fingolfin fingolfin closed this Apr 8, 2022
@fingolfin fingolfin reopened this Apr 8, 2022
@ChrisJefferson
Copy link
Contributor

Hi!

Thanks for the PR. Would you consider writing some tests for this function? It looks to me like there are currently no tests at all :(

Obviously within reason the more tests the better! But it's also fine to just put in some basic tests, and also some tests which cover the case you are changing. If you want some help writing tests please do ask, but as a quick suggestion, I'd look at some files in tst/opers, to get a feeling for what the tests should look like.

@RamonEstebanRomero
Copy link
Contributor Author

Hi @ChrisJefferson !

Thanks for the PR. Would you consider writing some tests for this function? It looks to me like there are currently no tests at all :(

Obviously within reason the more tests the better! But it's also fine to just put in some basic tests, and also some tests which cover the case you are changing. If you want some help writing tests please do ask, but as a quick suggestion, I'd look at some files in tst/opers, to get a feeling for what the tests should look like.

Fine, I will try to prepare some tests for this function during the next days with some examples, trivial cases, and different options. I remember some years ago I discovered a problem when applying this function to trivial groups, but I was not aware that with the retnorm option the output should also present the normalisers.

@fingolfin fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: library labels Apr 11, 2022
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.

Yes, I never considered the case of the trivial group in these cases, this should fix it.

The function `SubgroupsSolvableGroup` with a trivial group as argument
did not return the list of normalizers when called with the option `retnorm`.

Co-authored-by: Max Horn <[email protected]>
@fingolfin
Copy link
Member

I tweaked the formatting and commit message and added a test case.

@fingolfin fingolfin merged commit 5e8c0db into gap-system:master May 11, 2022
@fingolfin fingolfin changed the title Update trivial case for SubgroupsSolvableGroup with retnorm Update trivial case for SubgroupsSolvableGroup with retnorm Aug 17, 2022
@fingolfin fingolfin added kind: bug Issues describing general bugs, and PRs fixing them kind: bug: wrong result Issues describing bugs that result in mathematically or otherwise wrong results, and PRs fixing them release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes and removed kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements labels Aug 17, 2022
@fingolfin fingolfin changed the title Update trivial case for SubgroupsSolvableGroup with retnorm Fix SubgroupsSolvableGroup to honor retnorm option even for trivial groups Aug 17, 2022
@fingolfin fingolfin removed the kind: bug: wrong result Issues describing bugs that result in mathematically or otherwise wrong results, and PRs fixing them label Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Issues describing general bugs, and PRs fixing them release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants