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

Added IsNonabelianSimpleGroup; removed bad implication IsSimpleGroup => IsAlmostSimpleGroup #3522

Merged
merged 1 commit into from
Jun 27, 2019

Conversation

hulpke
Copy link
Contributor

@hulpke hulpke commented Jun 25, 2019

and use it in library wherever this is intended in place of IsSimpleGroup.

There were places in the code which uses IsSimpleGroup intending nonabelian simple. While thy did not cause obvious errors, it is cleaner to use a dedicated filter and will also inherit the IsAlmostSimpleGroup implication.

Release Notes:
Clarified definition and use of IsSimpleGroup to allow for abelian simple groups and introduced IsNonabelianSimpleGroup.

@hulpke hulpke added the release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes label Jun 25, 2019
@codecov
Copy link

codecov bot commented Jun 26, 2019

Codecov Report

Merging #3522 into master will decrease coverage by 12.01%.
The diff coverage is 53.12%.

@@             Coverage Diff             @@
##           master    #3522       +/-   ##
===========================================
- Coverage   85.45%   73.44%   -12.02%     
===========================================
  Files         696      534      -162     
  Lines      344887   267375    -77512     
===========================================
- Hits       294739   196370    -98369     
- Misses      50148    71005    +20857
Impacted Files Coverage Δ
lib/gpprmsya.gi 69.24% <0%> (-17.2%) ⬇️
lib/grpperm.gi 86.7% <0%> (-4.15%) ⬇️
lib/factgrp.gi 53.45% <0%> (-29.2%) ⬇️
lib/grp.gd 100% <100%> (ø) ⬆️
lib/maxsub.gi 34.5% <100%> (-32.08%) ⬇️
lib/clashom.gi 39.43% <100%> (-42.1%) ⬇️
lib/grplatt.gi 33.22% <22.22%> (-44.82%) ⬇️
lib/fitfree.gi 50.97% <50%> (-8.79%) ⬇️
lib/morpheus.gi 52.13% <60%> (-32.94%) ⬇️
lib/gpfpiso.gi 55.67% <66.66%> (-23.18%) ⬇️
... and 398 more

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Thanks, this is a good idea. I'd do it slightly differently, though, see suggestions.

lib/grp.gd Outdated Show resolved Hide resolved
lib/grp.gd Outdated Show resolved Hide resolved
lib/grp.gi Outdated Show resolved Hide resolved
lib/grpnice.gi Outdated Show resolved Hide resolved
lib/grp.gd Outdated Show resolved Hide resolved
lib/grplatt.gi Outdated Show resolved Hide resolved
@fingolfin fingolfin added release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes and removed release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Jun 26, 2019
lib/grplatt.gi Outdated Show resolved Hide resolved
lib/grplatt.gi Outdated Show resolved Hide resolved
lib/morpheus.gi Outdated Show resolved Hide resolved
lib/grplatt.gi Outdated Show resolved Hide resolved
@fingolfin
Copy link
Member

I fixed the diff in the manual, tests are running again

@wilfwilson
Copy link
Member

The Travis build has passed but GitHub doesn't seem to have realised, so I can't merge.

@coveralls
Copy link

coveralls commented Jun 27, 2019

Coverage Status

Coverage decreased (-0.002%) to 85.282% when pulling bd4f9a0 on hulpke:fixes into 662a665 on gap-system:master.

@hulpke hulpke force-pushed the fixes branch 2 times, most recently from eab81b1 to f9bf77e Compare June 27, 2019 14:59
@hulpke
Copy link
Contributor Author

hulpke commented Jun 27, 2019

I've included the last suggestion and rebases, lets hope (indeed it did) Travis complies.

and use it in library wherever this is intended in place of `IsSimpleGroup`.
@fingolfin fingolfin merged commit 8720ba1 into gap-system:master Jun 27, 2019
@fingolfin fingolfin changed the title ENHANCE: Added IsNonabelianSimpleGroup Added IsNonabelianSimpleGroup; removed bad implication IsSimpleGroup => IsAlmostSimpleGroup Jun 27, 2019
@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 kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements labels Jun 27, 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 20, 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: bug: wrong result Issues describing bugs that result in mathematically or otherwise wrong results, and PRs fixing them kind: bug Issues describing general bugs, and PRs fixing them 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants